[Libguestfs] [PATCH nbdkit 6/9] common/utils: Add a copy_environ utility function.
Eric Blake
eblake at redhat.com
Wed Apr 15 20:41:06 UTC 2020
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
More information about the Libguestfs
mailing list