[Outreachy]

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 <sarahjmi07@gmail.com>

----------

On 05/03/2016 07:45, Sarah Khan wrote:
> util/envlist.c:This patch replaces malloc with g_malloc

This should be placed in the subject.  Please follow the instructions at
http://wiki.qemu.org/Contribute/SubmitAPatch#Submitting_your_Patches to
format the patch correctly.

> -	if ((envlist = malloc(sizeof (*envlist))) == NULL)
> +	if ((envlist = g_malloc(sizeof (*envlist))) == NULL)
>  		return (NULL);

g_malloc will never return NULL, so you can remove the if.

> -	if ((entry = malloc(sizeof (*entry))) == NULL)
> +	if ((entry = g_malloc(sizeof (*entry))) == NULL)
>  		return (errno);

Likewise.

>  	if ((entry->ev_var = strdup(env)) == NULL) {

You could also replace strdup with g_strdup...

> -		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);

... because here you are replacing its freeing with g_free.

> +		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);

This if can be removed.

Note that you have included the patch twice in your message.

Thanks,

Paolo

Sarah Khan <sarahjmi07@gmail.com> 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 <sarahjmi07@gmail.com>

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