Re: [Qemu-devel] [PATCH][Outreachy]

[Alex Bennée]

Sarah Khan <address@hidden> writes:

> util/envlist.c:This patch replaces malloc with g_malloc
>
> This replacement was suggested as part of the bite-sized tasks.
>
> Signed-off-by: Sarah Khan <address@hidden>

Thanks for your contribution. A few notes for the next revision.

It's always worth running $SRC/scripts/checkpatch.pl to check for style
issues. While there are bits of the code base that don't conform we tend
to fix up style issues with the code that is being changed as we go.

>
> ----------
> diff --git a/util/envlist.c b/util/envlist.c
> index e86857e..0324fe2 100644
> --- a/util/envlist.c
> +++ b/util/envlist.c
> @@ -25,7 +25,7 @@ envlist_create(void)
>  {
>       envlist_t *envlist;
>
> -     if ((envlist = malloc(sizeof (*envlist))) == NULL)
> +     if ((envlist = g_malloc(sizeof (*envlist))) == NULL)
>               return (NULL);

g_malloc has different behaviour from malloc in so far as it never fails
(or rather if it does you program aborts). This means a lot of
boilerplate checking can be simplified.

>
>       QLIST_INIT(&envlist->el_entries);
> @@ -48,10 +48,10 @@ envlist_free(envlist_t *envlist)
>               entry = envlist->el_entries.lh_first;
>               QLIST_REMOVE(entry, ev_link);
>
> -             free((char *)entry->ev_var);
> -             free(entry);
> +             g_free((char *)entry->ev_var);
> +             g_free(entry);
>       }
> -     free(envlist);
> +     g_free(envlist);
>  }
>
>  /*
> @@ -155,16 +155,16 @@ envlist_setenv(envlist_t *envlist, const char *env)
>
>       if (entry != NULL) {
>               QLIST_REMOVE(entry, ev_link);
> -             free((char *)entry->ev_var);
> -             free(entry);
> +             g_free((char *)entry->ev_var);
> +             g_free(entry);
>       } else {
>               envlist->el_count++;
>       }
>
> -     if ((entry = malloc(sizeof (*entry))) == NULL)
> +     if ((entry = g_malloc(sizeof (*entry))) == NULL)
>               return (errno);
>       if ((entry->ev_var = strdup(env)) == NULL) {

If you're cleaning up the memory allocation you should probably also use
g_strdup() here for the string allocation (it gets g_free'd later).

> -             free(entry);
> +             g_free(entry);
>               return (errno);
>       }
>       QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link);
> @@ -201,8 +201,8 @@ envlist_unsetenv(envlist_t *envlist, const char *env)
>       }
>       if (entry != NULL) {
>               QLIST_REMOVE(entry, ev_link);
> -             free((char *)entry->ev_var);
> -             free(entry);
> +             g_free((char *)entry->ev_var);
> +             g_free(entry);
>
>               envlist->el_count--;
>       }
> @@ -225,7 +225,7 @@ envlist_to_environ(const envlist_t *envlist, size_t 
> *count)
>       struct envlist_entry *entry;
>       char **env, **penv;
>
> -     penv = env = malloc((envlist->el_count + 1) * sizeof (char *));
> +     penv = env = g_malloc((envlist->el_count + 1) * sizeof (char *));
>       if (env == NULL)
>               return (NULL);

Again this checking is now redundant.

> ---
>  util/envlist.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/util/envlist.c b/util/envlist.c
> index e86857e..0324fe2 100644
> --- a/util/envlist.c
> +++ b/util/envlist.c
> @@ -25,7 +25,7 @@ envlist_create(void)
>  {
>       envlist_t *envlist;
>
> -     if ((envlist = malloc(sizeof (*envlist))) == NULL)
> +     if ((envlist = g_malloc(sizeof (*envlist))) == NULL)
>               return (NULL);
>
>       QLIST_INIT(&envlist->el_entries);
> @@ -48,10 +48,10 @@ envlist_free(envlist_t *envlist)
>               entry = envlist->el_entries.lh_first;
>               QLIST_REMOVE(entry, ev_link);
>
> -             free((char *)entry->ev_var);
> -             free(entry);
> +             g_free((char *)entry->ev_var);
> +             g_free(entry);
>       }
> -     free(envlist);
> +     g_free(envlist);
>  }
>
>  /*
> @@ -155,16 +155,16 @@ envlist_setenv(envlist_t *envlist, const char *env)
>
>       if (entry != NULL) {
>               QLIST_REMOVE(entry, ev_link);
> -             free((char *)entry->ev_var);
> -             free(entry);
> +             g_free((char *)entry->ev_var);
> +             g_free(entry);
>       } else {
>               envlist->el_count++;
>       }
>
> -     if ((entry = malloc(sizeof (*entry))) == NULL)
> +     if ((entry = g_malloc(sizeof (*entry))) == NULL)
>               return (errno);
>       if ((entry->ev_var = strdup(env)) == NULL) {
> -             free(entry);
> +             g_free(entry);
>               return (errno);
>       }
>       QLIST_INSERT_HEAD(&envlist->el_entries, entry, ev_link);
> @@ -201,8 +201,8 @@ envlist_unsetenv(envlist_t *envlist, const char *env)
>       }
>       if (entry != NULL) {
>               QLIST_REMOVE(entry, ev_link);
> -             free((char *)entry->ev_var);
> -             free(entry);
> +             g_free((char *)entry->ev_var);
> +             g_free(entry);
>
>               envlist->el_count--;
>       }
> @@ -225,7 +225,7 @@ envlist_to_environ(const envlist_t *envlist, size_t 
> *count)
>       struct envlist_entry *entry;
>       char **env, **penv;
>
> -     penv = env = malloc((envlist->el_count + 1) * sizeof (char *));
> +     penv = env = g_malloc((envlist->el_count + 1) * sizeof (char *));
>       if (env == NULL)
>               return (NULL);

Same here.

Thanks for you submission. Feel free to CC me on your next version.

--
Alex Bennée