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

[Richard W.M. Jones]

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