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 _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
