some g_autofree conversions

[neheb]

Signed-off-by: Rosen Penev rosenp@gmail.com

No idea if this is even desired. It sort of requires goto removals. It also doesn't work in loops. clang-analyzer also seems to get confused.

I think @atheik will want gchar removals. There's more opportunity to do this elsewhere.

[atheik]

@neheb,

For commit b0226d5, please prefix the commit title with ksmbd-tools:, i.e. ksmbd-tools: some g_autofree conversions, and write a commit message. Also, please see https://github.com/cifsd-team/ksmbd-tools/pull/282#issuecomment-1269203400. To summarize, do not mix declarations and code, and when you have moved the g_autofree variable declarations back to the top of the block, always initialize them to NULL.

[neheb]

right. it's cleaner and less error prone to mix declarations and code. But I guess that's up to @namjaejeon .

[atheik]

@neheb,

right. it's cleaner and less error prone to mix declarations and code. But I guess that's up to @namjaejeon .

That is entirely a matter of opinion. Just as many, if not more, would argue that declarations at the top of the block is clearer. There is no reason to suddenly break this convention for the ksmbd-tools codebase.

[atheik]

@neheb,

right. it's cleaner and less error prone to mix declarations and code. But I guess that's up to @namjaejeon .

Also, like I explained in https://github.com/cifsd-team/ksmbd-tools/pull/282#issuecomment-1269203400, mixed declarations and code combined with the use of g_autofree, i.e. __attribute__((cleanup)), make it easy to write buggy code that looks alright at a glance.

Consider, for example, that the following calls g_free() for uninitialized memory:

cat <<'EOF' >a.c
#include <glib.h>
#include <errno.h>

int create_context()
{
    return 0;
}

int some_fun()
{
    return -EINVAL;
}

int destroy_context()
{
    return 0;
}

int main(void)
{
    create_context();

    if (some_fun())
        goto out;

    g_autofree char *cp = g_strdup("Hello!");
    g_print("%s\n", cp);

out:
    destroy_context();
    return 0;
}
EOF

gcc a.c -fanalyzer -g -O0 $(pkg-config --cflags --libs glib-2.0)
valgrind --leak-check=full ./a.out

By moving the declaration of cp to the top of the block and initializing it NULL, this type of bug can be avoided.

[neheb]

Right. I mentioned it doesn't work right with goto.

[atheik]

@neheb,

Right. I mentioned it doesn't work right with goto.

As long as you declare and initialize to NULL at the top of the block, it does work right with goto. With some variable scope adjustments or intermediate frees (e.g. g_free(ptr); ptr = NULL), the same is true for loops.

[neheb]

yeah but the whole goal is to simplify the code.

[atheik]

@neheb,

yeah but the whole goal is to simplify the code.

I do not understand what you mean by this. Could you please elaborate?

Here's an example of moving the declaration to the top of the block and initializing to NULL:

diff --git a/addshare/share_admin.c b/addshare/share_admin.c
index 8e9ced5..852a538 100644
--- a/addshare/share_admin.c
+++ b/addshare/share_admin.c
@@ -138,12 +138,14 @@ static void update_share_cb(gpointer key,

 int command_add_share(char *smbconf, char *name, char *opts)
 {
+   g_autofree char *new_name = NULL;
+
    if (g_hash_table_lookup(parser.groups, name)) {
        pr_err("Share `%s' already exists\n", name);
        return -EEXIST;
    }

-   g_autofree char *new_name = new_group_name(name);
+   new_name = new_group_name(name);
    if (cp_parse_external_smbconf_group(new_name, opts))
        return -EINVAL;

Here's an example of it working with loops by changing the variable scope to a deeper block:

diff --git a/mountd/mountd.c b/mountd/mountd.c
index 667a198..a447ce4 100644
--- a/mountd/mountd.c
+++ b/mountd/mountd.c
@@ -84,7 +84,6 @@ static int show_version(void)
 static int create_lock_file(void)
 {
    int ret = -EINVAL;
-   char *open_m = NULL;
    char pid_buf[10];
    size_t sz;

@@ -92,7 +91,7 @@ retry:
    lock_fd = open(KSMBD_LOCK_FILE, O_CREAT | O_EXCL | O_WRONLY,
            S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH);
    if (lock_fd < 0) {
-       open_m = g_strdup_printf("%m");
+       g_autofree char *open_m = g_strdup_printf("%m");

        if (send_signal_to_ksmbd_mountd(0) == -ESRCH) {
            pr_info("Unlinking orphaned lock file\n");
@@ -106,8 +105,6 @@ retry:
            goto out;
        }

-       g_free(open_m);
-       open_m = NULL;
        goto retry;
    }

@@ -124,7 +121,6 @@ retry:

    ret = 0;
 out:
-   g_free(open_m);
    return ret;
 }

[neheb]

Addressed most mixed decl and assignment. Can't say I like it.

[atheik]

@neheb,

Addressed most mixed decl and assignment. Can't say I like it.

Please elaborate and help me understand your point of view.

[neheb]

first of all it's double assignment:

x = NULL;
x = y;

I understand that compilers eliminate it but still.

It's also a bit weird to declare something even when there's an early return. eg:

int a, b = 0;
if  (!b)
   return 0;
a = x;

[namjaejeon]

right. it's cleaner and less error prone to mix declarations and code. But I guess that's up to @namjaejeon .

I agreed with @atheik opinion.

[neheb]

The PR was updated with that change.

[atheik]

@neheb,

Readability trumps very minor optimizations like that. And, like you said, the compiler optimizes it anyway.

[namjaejeon]

@neheb If you update patch description(commit message) and prefix(ksmbd-tools:), I will apply this patch:) Thanks!

[neheb]

quick question: while looking for more opportunities, I noticed this:

--- a/tools/config_parser.c
+++ b/tools/config_parser.c
@@ -86,7 +86,7 @@ static int add_new_group(char *line)
 {
        char *begin = line;
        char *end = line;
-       char *name = NULL;
+       g_autofree char *name = NULL;
        struct smbconf_group *group = NULL;
        struct smbconf_group *lookup;

@@ -95,13 +95,12 @@ static int add_new_group(char *line)

        name = g_strndup(begin + 1, end - begin - 1);
        if (!name)
-               goto out_free;
+               return -ENOMEM;

        lookup = g_hash_table_lookup(parser.groups, name);
        if (lookup) {
                parser.current = lookup;
                pr_info("Multiple definitions for group `%s'\n", name);
-               g_free(name);
                return 0;
        }

@@ -112,19 +111,14 @@ static int add_new_group(char *line)
                                          g_str_equal,
                                          kv_release_cb,
                                          kv_release_cb);
-       if (!group->kv)
-               goto out_free;
+       if (!group->kv) {
+               g_free(group);
+               return -ENOMEM;
+       }

        parser.current = group;
        g_hash_table_insert(parser.groups, group->name, group);
        return 0;
-
-out_free:
-       g_free(name);
-       if (group && group->kv)
-               g_hash_table_destroy(group->kv);
-       g_free(group);
-       return -ENOMEM;
 }

 static int add_group_key_value(char *line)

So for the first goto, group is NULL so g_hash_table_destroy is not called. In the second, group->kv is NULL and it is also not called. Is this a bug or am I understanding it wrong?

[atheik]

@neheb,

That OOM error path is not used regardless, please see the last paragraph of https://github.com/cifsd-team/ksmbd-tools/pull/282#issuecomment-1270163074.

group is never NULL because g_malloc() aborts on OOM.

    group = g_malloc(sizeof(struct smbconf_group));

group->kv is never NULL because g_hash_table_new_full() aborts on OOM:

    group->kv = g_hash_table_new_full(g_str_hash,

I have done some GLib related cleanup here: https://github.com/atheik/ksmbd-tools/commits/remove-unused-oom-error-paths-for-glib-calls

[neheb]

Hmm OK. glib2 cleanups look good.

[namjaejeon]

I will apply this patch. Thanks!

[namjaejeon]

Applied. Thanks!