Re: [Libguestfs] [PATCH nbdkit 6/9] common/utils: Add a copy_environ utility function.
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