3:41 p.m.
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