Re: [RFC v4 03/11] linux-user: Implement envlist_appendenv and add tests for envlist

["Alex Bennée" <alex.bennee@linaro.org>]

Yeqi Fu <fufuyqqqqqq@gmail.com> writes:

> Signed-off-by: Yeqi Fu <fufuyqqqqqq@gmail.com>
> ---
>  include/qemu/envlist.h    | 13 ++++++
>  tests/unit/meson.build    |  1 +
>  tests/unit/test-envlist.c | 94 +++++++++++++++++++++++++++++++++++++++
>  util/envlist.c            | 71 ++++++++++++++++++++++++-----
>  4 files changed, 169 insertions(+), 10 deletions(-)
>  create mode 100644 tests/unit/test-envlist.c

Thanks for adding the unit test.
>  
> +/*
> + * Appends environment value to envlist. If the environment
> + * variable already exists, the new value is appended to the
> + * existing one.
> + *
> + * Returns 0 in success, errno otherwise.
> + */
> +int
> +envlist_appendenv(envlist_t *envlist, const char *env, const char *separator)
> +{
> +    struct envlist_entry *entry = NULL;
> +    const char *eq_sign;
> +    size_t envname_len;
> +
> +    if ((envlist == NULL) || (env == NULL) || (separator == NULL)) {
> +        return -EINVAL;
> +    }
> +
> +    /* find out first equals sign in given env */
> +    eq_sign = strchr(env, '=');
> +    if (eq_sign == NULL) {
> +        return -EINVAL;
> +    }
> +
> +    if (strchr(eq_sign + 1, '=') != NULL) {
> +        return -EINVAL;
> +    }
> +
> +    envname_len = eq_sign - env + 1;
> +
> +    /*
> +     * If there already exists variable with given name,
> +     * we append the new value to the existing one.
> +     */
> +    for (entry = envlist->el_entries.lh_first; entry != NULL;
> +        entry = entry->ev_link.le_next) {
> +        if (strncmp(entry->ev_var, env, envname_len) == 0) {
> +            break;
> +        }
> +    }
> +
> +    if (entry != NULL) {
> +        char *new_env_value = NULL;
> +        size_t new_env_len = strlen(entry->ev_var) + strlen(eq_sign)
> +            + strlen(separator) + 1;
> +        new_env_value = g_malloc(new_env_len);
> +        strcpy(new_env_value, entry->ev_var);
> +        strcat(new_env_value, separator);
> +        strcat(new_env_value, eq_sign + 1);
> +        g_free((char *)entry->ev_var);
> +        entry->ev_var = new_env_value;

You could probably avoid messing about with strlens if you used the
handy glib functions:

    if (entry != NULL) {
        GString *new_val = g_string_new(entry->ev_var);
        g_string_append(new_val, separator);
        g_string_append(new_val, eq_sign + 1);
        g_free(entry->ev_var);
        entry->ev_var = g_string_free(new_val, false);
    } else {

Also the fact you needed to cast entry->ev_var to g_free() should have
pointed out that now we can change env entries we need to drop the const
from the char *ev_var; definition in envlist_entry.

Otherwise with those fixes:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro