[Libguestfs] [PATCH nbdkit 6/9] common/utils: Add a copy_environ utility function.

[Eric Blake]

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> This allows you to copy an environ, while optionally adding extra
> (key, value) pairs to it.
> ---
>   common/utils/Makefile.am |   2 +
>   common/utils/utils.h     |   1 +
>   common/utils/environ.c   | 116 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 119 insertions(+)
> 

> +++ b/common/utils/utils.h
> @@ -38,5 +38,6 @@ extern void uri_quote (const char *str, FILE *fp);
>   extern int exit_status_to_nbd_error (int status, const char *cmd);
>   extern int set_cloexec (int fd);
>   extern int set_nonblock (int fd);
> +extern char **copy_environ (char **env, ...);

Should use __attribute__((sentinel)), to let the compiler enforce that 
we don't forget a trailing NULL.

> +++ b/common/utils/environ.c

> +
> +DEFINE_VECTOR_TYPE(environ_t, char *);
> +
> +/* Copy an environ.  Also this allows you to add (key, value) pairs to
> + * the environ through the varargs NULL-terminated list.  Returns NULL
> + * if the copy or allocation failed.
> + */
> +char **
> +copy_environ (char **env, ...)
> +{
> +  environ_t ret = empty_vector;
> +  size_t i, len;
> +  char *s;
> +  va_list argp;
> +  const char *key, *value;
> +
> +  /* Copy the existing keys into the new vector. */
> +  for (i = 0; env[i] != NULL; ++i) {
> +    s = strdup (env[i]);
> +    if (s == NULL) {
> +      nbdkit_error ("strdup: %m");
> +      goto error;
> +    }
> +    if (environ_t_append (&ret, s) == -1) {
> +      nbdkit_error ("realloc: %m");
> +      goto error;
> +    }
> +  }
> +
> +  /* Add the new keys. */
> +  va_start (argp, env);
> +  while ((key = va_arg (argp, const char *)) != NULL) {
> +    value = va_arg (argp, const char *);
> +    if (asprintf (&s, "%s=%s", key, value) == -1) {
> +      nbdkit_error ("asprintf: %m");
> +      goto error;
> +    }
> +
> +    /* Search for key in the existing environment.  It's O(n^2) ... */

Unfortunately true. There's no guarantee that the original environ is 
sorted to make this more efficient, but the overhead of storing our 
replacement in a hash just to avoid the O(n^2) seems wasted.  That 
argues that we should try to avoid invoking copy_environ in hot-spots 
(for example, in the sh plugin, can we get away with doing it once in 
.get_ready, rather than before every single callback?)

hmm - looking at patch 8, you delayed things because of --run.  We 
already know we have to call .config_complete before run_command(), but 
would it hurt if we reordered things to call run_command() prior to 
.get_ready?)

> +    len = strlen (key);
> +    for (i = 0; i < ret.size; ++i) {
> +      if (strncmp (key, ret.ptr[i], len) == 0 && ret.ptr[i][len] == '=') {
> +        /* Replace the existing key. */
> +        free (ret.ptr[i]);
> +        ret.ptr[i] = s;
> +        goto found;
> +      }
> +    }
> +
> +    /* Else, append a new key. */
> +    if (environ_t_append (&ret, s) == -1) {
> +      nbdkit_error ("realloc: %m");
> +      goto error;
> +    }
> +
> +  found: ;
> +  }

Odd to see a label for an empty statement, but I don't see any more 
concise way to do the flow control you need.

> +  va_end (argp);

Ouch - you skip va_end() on some error paths.  On most systems, this is 
harmless, but POSIX says that va_start() without an unmatched va_end() 
is undefined behavior (a memory leak on some platforms)

> +
> +  /* Append a NULL pointer. */
> +  if (environ_t_append (&ret, NULL) == -1) {
> +    nbdkit_error ("realloc: %m");
> +    goto error;
> +  }
> +
> +  /* Return the list of strings. */
> +  return ret.ptr;
> +
> + error:
> +  environ_t_iter (&ret, (void *) free);
> +  free (ret.ptr);
> +  return NULL;
> +}
> 

Otherwise this looks like a nice addition.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org