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

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

On Wed, Apr 15, 2020 at 03:41:06PM -0500, Eric Blake
wrote:
> 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.
Ooops ... I had this in an earlier version and somehow
managed to drop it :-(  I think when it was originally
in a separate header file.

Fixed my copy.
> 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?)
Yes we can do it once in .get_ready, to add tmpdir (which never
changes).  If we need to add further environment variables that change
on each script invocation then we'd do it a second time in the
function.

It's fine to do it in .get_ready because we would still store it in a
char **env variable, rather than updating the global environ or
calling setenv.

I'll update my version.
> >+  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)
Yeah I was wondering about this ...  I'll try to fix it.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

On Wed, Apr 15, 2020 at 09:54:47PM +0100, Richard W.M. Jones
wrote:
> Yes we can do it once in .get_ready, to add tmpdir (which never
> changes).  If we need to add further environment variables that change
> on each script invocation then we'd do it a second time in the
> function.
> 
> It's fine to do it in .get_ready because we would still store it in a
> char **env variable, rather than updating the global environ or
> calling setenv.
Actually, I mean do it in .load, but still instead of calling
setenv we'd update a global char **env variable.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW