Libguestfs - Apr 2020 - Re: [PATCH nbdkit 1/9] common: Add a generic implementation of vectors.

This was a rather longer trip around the houses than I anticipated!

The basic purpose of the patch series is to set $nbdkit_stdio_safe to
"0" or "1" in sh and eval plugin scripts.

To do that, I ended up adding a nicer way to manipulate environ lists,
and to do that, I ended up adding a whole generic vector
implementation which is applicable in a lot of different places.

Rich.

Can be used for building up lists of things, especially
lists of strings.
---
 common/include/Makefile.am   |  6 +++
 common/include/test-vector.c | 90 +++++++++++++++++++++++++++++++++
 common/include/vector.h      | 96 ++++++++++++++++++++++++++++++++++++
 .gitignore                   |  1 +
 4 files changed, 193 insertions(+)

diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index 4482de37..3089a0a1 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -45,6 +45,7 @@ EXTRA_DIST = \
 	random.h \
 	rounding.h \
 	tvdiff.h \
+	vector.h \
 	$(NULL)
 
 # Unit tests.
@@ -59,6 +60,7 @@ TESTS = \
 	test-nextnonzero \
 	test-random \
 	test-tvdiff \
+	test-vector \
 	$(NULL)
 check_PROGRAMS = $(TESTS)
 
@@ -97,3 +99,7 @@ test_random_CFLAGS = $(WARNINGS_CFLAGS)
 test_tvdiff_SOURCES = test-tvdiff.c tvdiff.h
 test_tvdiff_CPPFLAGS = -I$(srcdir)
 test_tvdiff_CFLAGS = $(WARNINGS_CFLAGS)
+
+test_vector_SOURCES = test-vector.c vector.h
+test_vector_CPPFLAGS = -I$(srcdir)
+test_vector_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/include/test-vector.c b/common/include/test-vector.c
new file mode 100644
index 00000000..8bf6e349
--- /dev/null
+++ b/common/include/test-vector.c
@@ -0,0 +1,90 @@
+/* nbdkit
+ * Copyright (C) 2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <assert.h>
+
+#include "vector.h"
+
+DEFINE_VECTOR_TYPE(int64_vector, int64_t);
+DEFINE_VECTOR_TYPE(string_vector, char *);
+
+static void
+test_int64_vector (void)
+{
+  int64_vector v = empty_vector;
+  size_t i;
+
+  for (i = 0; i < 10; ++i)
+    assert (int64_vector_append (&v, i) == 0);
+  for (i = 0; i < 10; ++i)
+    assert (v.ptr[i] == i);
+  free (v.ptr);
+}
+
+static void
+test_string_vector (void)
+{
+  string_vector v = empty_vector;
+  size_t i;
+
+  for (i = 0; i < 10; ++i) {
+    char *s;
+
+    assert (asprintf (&s, "number %zu", i) >= 0);
+    assert (string_vector_append (&v, s) == 0);
+  }
+  /* NULL-terminate the strings. */
+  assert (string_vector_append (&v, NULL) == 0);
+
+  /* Now print them. */
+  for (i = 0; v.ptr[i] != NULL; ++i)
+    printf ("%s\n", v.ptr[i]);
+  assert (i == 10);
+
+  /* And free them.  We can use the generated iter function here
+   * even though it calls free on the final NULL pointer.
+   */
+  string_vector_iter (&v, (void*)free);
+  free (v.ptr);
+}
+
+int
+main (int argc, char *argv[])
+{
+  test_int64_vector ();
+  test_string_vector ();
+}
diff --git a/common/include/vector.h b/common/include/vector.h
new file mode 100644
index 00000000..69a350be
--- /dev/null
+++ b/common/include/vector.h
@@ -0,0 +1,96 @@
+/* nbdkit
+ * Copyright (C) 2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/* Simple implementation of appendable vector.  There are two main
+ * use-cases we consider: lists of strings (either with a defined
+ * length, or NULL-terminated), and lists of numbers.  It is generic
+ * so could be used for lists of anything (eg. structs) where being
+ * able to append easily is important.
+ */
+
+#ifndef NBDKIT_VECTOR_H
+#define NBDKIT_VECTOR_H
+
+#include <assert.h>
+
+#define DEFINE_VECTOR_TYPE(name, type)                                  \
+  struct name {                                                         \
+    type *ptr;                 /* Pointer to array of items. */         \
+    size_t size;               /* Number of valid items in the array. */ \
+    size_t alloc;              /* Number of items allocated. */         \
+  };                                                                    \
+  typedef struct name name;                                             \
+  static inline int                                                     \
+  name##_extend (name *v, size_t n)                                     \
+  {                                                                     \
+    return generic_vector_extend ((struct generic_vector *)v, n,        \
+                                  sizeof (type));                       \
+  }                                                                     \
+  static inline int                                                     \
+  name##_append (name *v, type elem)                                    \
+  {                                                                     \
+    if (v->size >= v->alloc) {                                        
\
+      if (name##_extend (v, 1) == -1) return -1;                        \
+    }                                                                   \
+    v->ptr[v->size++] = elem;                                           \
+    return 0;                                                           \
+  }                                                                     \
+  static inline void                                                    \
+  name##_iter (name *v, void (*f) (type elem))                          \
+  {                                                                     \
+    size_t i;                                                           \
+    for (i = 0; i < v->size; ++i)                                       \
+      f (v->ptr[i]);                                                    \
+  }
+
+#define empty_vector { .ptr = NULL, .size = 0, .alloc = 0 }
+
+struct generic_vector {
+  void *ptr;
+  size_t size;
+  size_t alloc;
+};
+
+static int
+generic_vector_extend (struct generic_vector *v, size_t n, size_t itemsize)
+{
+  void *newptr;
+
+  newptr = realloc (v->ptr, (n + v->alloc) * itemsize);
+  if (newptr == NULL)
+    return -1;
+  v->ptr = newptr;
+  v->alloc += n;
+  return 0;
+}
+
+#endif /* NBDKIT_VECTOR_H */
diff --git a/.gitignore b/.gitignore
index c44fb40d..8974b64f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -42,6 +42,7 @@ plugins/*/*.3
 /common/include/test-nextnonzero
 /common/include/test-random
 /common/include/test-tvdiff
+/common/include/test-vector
 /common/protocol/generate-protostrings.sh
 /common/protocol/protostrings.c
 /common/utils/test-quotes
-- 
2.25.0

These plugins have in common that they store either a list of
allocated strings or a list of constant strings.  Define either
string_vector or const_string_vector as appropriate and use it to
store these lists.
---
 plugins/iso/Makefile.am         |  1 +
 plugins/split/Makefile.am       |  1 +
 plugins/floppy/virtual-floppy.h |  7 +++--
 plugins/floppy/directory-lfn.c  | 35 +++++++++-------------
 plugins/floppy/virtual-floppy.c | 11 +++----
 plugins/iso/iso.c               | 25 ++++++----------
 plugins/split/split.c           | 51 +++++++++++++++------------------
 plugins/ssh/ssh.c               | 24 +++++++---------
 8 files changed, 69 insertions(+), 86 deletions(-)

diff --git a/plugins/iso/Makefile.am b/plugins/iso/Makefile.am
index a0fd337a..d20e1768 100644
--- a/plugins/iso/Makefile.am
+++ b/plugins/iso/Makefile.am
@@ -43,6 +43,7 @@ nbdkit_iso_plugin_la_SOURCES = \
 	$(NULL)
 
 nbdkit_iso_plugin_la_CPPFLAGS = \
+	-I$(top_srcdir)/common/include \
 	-I$(top_srcdir)/common/utils \
 	-I$(top_srcdir)/include \
 	-I. \
diff --git a/plugins/split/Makefile.am b/plugins/split/Makefile.am
index d8218b9a..14af56f1 100644
--- a/plugins/split/Makefile.am
+++ b/plugins/split/Makefile.am
@@ -42,6 +42,7 @@ nbdkit_split_plugin_la_SOURCES = \
 
 nbdkit_split_plugin_la_CPPFLAGS = \
 	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/include \
 	-I$(top_srcdir)/common/utils \
 	$(NULL)
 nbdkit_split_plugin_la_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/plugins/floppy/virtual-floppy.h b/plugins/floppy/virtual-floppy.h
index 9617d386..30643df4 100644
--- a/plugins/floppy/virtual-floppy.h
+++ b/plugins/floppy/virtual-floppy.h
@@ -37,6 +37,7 @@
 #include <sys/stat.h>
 
 #include "regions.h"
+#include "vector.h"
 
 struct partition_entry {
   uint8_t bootable;             /* 0x00 or 0x80 if bootable */
@@ -130,6 +131,9 @@ struct dir_entry {
   uint32_t size;                /* 0x1C - file size */
 } __attribute__((packed));
 
+/* Appendable list of struct dir_entry. */
+DEFINE_VECTOR_TYPE(dir_entries, struct dir_entry);
+
 /* On disk directory entry (LFN). */
 struct lfn_entry {
   uint8_t seq;                  /* sequence number */
@@ -162,8 +166,7 @@ struct dir {
   size_t nr_files;
 
   /* On disk directory table. */
-  struct dir_entry *table;
-  size_t table_entries;
+  dir_entries table;
 };
 
 struct virtual_floppy {
diff --git a/plugins/floppy/directory-lfn.c b/plugins/floppy/directory-lfn.c
index 10caf84b..e06680e9 100644
--- a/plugins/floppy/directory-lfn.c
+++ b/plugins/floppy/directory-lfn.c
@@ -71,7 +71,7 @@ static void set_times (const struct stat *statbuf, struct
dir_entry *entry);
 static int convert_long_file_names (struct lfn *lfns, size_t n);
 static int convert_to_utf16le (const char *name, char **out, size_t
*output_len);
 static void free_lfns (struct lfn *lfns, size_t n);
-static ssize_t extend_dir_table (size_t di, struct virtual_floppy *floppy);
+static ssize_t append_dir_table (size_t di, const struct dir_entry *entry,
struct virtual_floppy *floppy);
 
 /* Create the on disk directory table for dirs[di]. */
 int
@@ -178,10 +178,9 @@ add_volume_label (const char *label, size_t di, struct
virtual_floppy *floppy)
   pad_string (label, 11, entry.name);
   entry.attributes = DIR_ENTRY_VOLUME_LABEL; /* Same as dosfstools. */
 
-  i = extend_dir_table (di, floppy);
+  i = append_dir_table (di, &entry, floppy);
   if (i == -1)
     return -1;
-  floppy->dirs[di].table[i] = entry;
   return 0;
 }
 
@@ -199,10 +198,9 @@ add_dot_entries (size_t di, struct virtual_floppy *floppy)
   entry.attributes = DIR_ENTRY_SUBDIRECTORY;
   set_times (&floppy->dirs[di].statbuf, &entry);
 
-  i = extend_dir_table (di, floppy);
+  i = append_dir_table (di, &entry, floppy);
   if (i == -1)
     return -1;
-  floppy->dirs[di].table[i] = entry;
 
   memset (&entry, 0, sizeof entry);
   pad_string ("..", 11, entry.name);
@@ -210,10 +208,9 @@ add_dot_entries (size_t di, struct virtual_floppy *floppy)
   pdi = floppy->dirs[di].pdi;
   set_times (&floppy->dirs[pdi].statbuf, &entry);
 
-  i = extend_dir_table (di, floppy);
+  i = append_dir_table (di, &entry, floppy);
   if (i == -1)
     return -1;
-  floppy->dirs[di].table[i] = entry;
 
   return 0;
 }
@@ -288,10 +285,9 @@ add_directory_entry (const struct lfn *lfn,
     memcpy (lfn_entry.name2, &s[5], 6*2);
     memcpy (lfn_entry.name3, &s[11], 2*2);
 
-    i = extend_dir_table (di, floppy);
+    i = append_dir_table (di, (const struct dir_entry *) &lfn_entry,
floppy);
     if (i == -1)
       return -1;
-    memcpy (&floppy->dirs[di].table[i], &lfn_entry, sizeof (struct
dir_entry));
   }
 
   /* Create the 8.3 (short name / DOS-compatible) entry. */
@@ -305,10 +301,9 @@ add_directory_entry (const struct lfn *lfn,
    * update_directory_first_cluster.
    */
 
-  i = extend_dir_table (di, floppy);
+  i = append_dir_table (di, &entry, floppy);
   if (i == -1)
     return -1;
-  floppy->dirs[di].table[i] = entry;
 
   return 0;
 }
@@ -530,22 +525,18 @@ free_lfns (struct lfn *lfns, size_t n)
   free (lfns);
 }
 
-/* Extend dirs[di].table by 1 directory entry. */
+/* Append entry to dirs[di].table.  Returns the index of the new entry. */
 static ssize_t
-extend_dir_table (size_t di, struct virtual_floppy *floppy)
+append_dir_table (size_t di, const struct dir_entry *entry,
+                  struct virtual_floppy *floppy)
 {
-  struct dir_entry *p;
   size_t i;
 
-  i = floppy->dirs[di].table_entries;
-  p = realloc (floppy->dirs[di].table, sizeof (struct dir_entry) * (i+1));
-  if (p == NULL) {
+  i = floppy->dirs[di].table.size;
+  if (dir_entries_append (&floppy->dirs[di].table, *entry) == -1) {
     nbdkit_error ("realloc: %m");
     return -1;
   }
-  floppy->dirs[di].table = p;
-  floppy->dirs[di].table_entries++;
-  memset (&floppy->dirs[di].table[i], 0, sizeof (struct dir_entry));
   return i;
 }
 
@@ -570,8 +561,8 @@ update_directory_first_cluster (size_t di, struct
virtual_floppy *floppy)
    * table entries.
    */
   i = 0;
-  for (j = 0; j < floppy->dirs[di].table_entries; ++j) {
-    entry = &floppy->dirs[di].table[j];
+  for (j = 0; j < floppy->dirs[di].table.size; ++j) {
+    entry = &floppy->dirs[di].table.ptr[j];
 
     /* Skip LFN entries. */
     if (entry->attributes == 0xf)
diff --git a/plugins/floppy/virtual-floppy.c b/plugins/floppy/virtual-floppy.c
index fc0cafa8..ad192976 100644
--- a/plugins/floppy/virtual-floppy.c
+++ b/plugins/floppy/virtual-floppy.c
@@ -117,7 +117,7 @@ create_virtual_floppy (const char *dir, const char *label,
   for (i = 0; i < floppy->nr_dirs; ++i) {
     floppy->dirs[i].first_cluster = cluster;
     nr_bytes -      ROUND_UP (floppy->dirs[i].table_entries * sizeof (struct
dir_entry),
+      ROUND_UP (floppy->dirs[i].table.size * sizeof (struct dir_entry),
                 CLUSTER_SIZE);
     floppy->data_size += nr_bytes;
     nr_clusters = nr_bytes / CLUSTER_SIZE;
@@ -221,7 +221,7 @@ free_virtual_floppy (struct virtual_floppy *floppy)
     free (floppy->dirs[i].name);
     free (floppy->dirs[i].subdirs);
     free (floppy->dirs[i].files);
-    free (floppy->dirs[i].table);
+    free (floppy->dirs[i].table.ptr);
   }
   free (floppy->dirs);
 }
@@ -677,14 +677,15 @@ create_regions (struct virtual_floppy *floppy)
     /* Directories can never be completely empty because of the volume
      * label (root) or "." and ".." entries (non-root).
      */
-    assert (floppy->dirs[i].table_entries > 0);
+    assert (floppy->dirs[i].table.size > 0);
 
     if (append_region_len (&floppy->regions,
                            i == 0 ? "root directory" :
floppy->dirs[i].name,
-                           floppy->dirs[i].table_entries *
+                           floppy->dirs[i].table.size *
                            sizeof (struct dir_entry),
                            0, CLUSTER_SIZE,
-                           region_data, (void *) floppy->dirs[i].table) ==
-1)
+                           region_data,
+                           (void *) floppy->dirs[i].table.ptr) == -1)
       return -1;
   }
 
diff --git a/plugins/iso/iso.c b/plugins/iso/iso.c
index 92554ace..733bcb60 100644
--- a/plugins/iso/iso.c
+++ b/plugins/iso/iso.c
@@ -43,10 +43,11 @@
 
 #include "cleanup.h"
 #include "utils.h"
+#include "vector.h"
 
 /* List of directories parsed from the command line. */
-static char **dirs = NULL;
-static size_t nr_dirs = 0;
+DEFINE_VECTOR_TYPE(string_vector, char *);
+static string_vector dirs = empty_vector;
 
 /* genisoimage or mkisofs program, picked at compile time, but can be
  * overridden at run time.
@@ -98,9 +99,9 @@ make_iso (void)
   fprintf (fp, " -quiet");
   if (params)
     fprintf (fp, " %s", params);
-  for (i = 0; i < nr_dirs; ++i) {
+  for (i = 0; i < dirs.size; ++i) {
     fputc (' ', fp);
-    shell_quote (dirs[i], fp);
+    shell_quote (dirs.ptr[i], fp);
   }
   /* Redirect output to the temporary file. */
   fprintf (fp, " >&%d", fd);
@@ -122,11 +123,8 @@ make_iso (void)
 static void
 iso_unload (void)
 {
-  size_t i;
-
-  for (i = 0; i < nr_dirs; ++i)
-    free (dirs[i]);
-  free (dirs);
+  string_vector_iter (&dirs, (void *) free);
+  free (dirs.ptr);
 
   if (fd >= 0)
     close (fd);
@@ -135,7 +133,6 @@ iso_unload (void)
 static int
 iso_config (const char *key, const char *value)
 {
-  char **new_dirs;
   char *dir;
 
   if (strcmp (key, "dir") == 0) {
@@ -143,15 +140,11 @@ iso_config (const char *key, const char *value)
     if (dir == NULL)
       return -1;
 
-    new_dirs = realloc (dirs, sizeof (char *) * (nr_dirs + 1));
-    if (new_dirs == NULL) {
+    if (string_vector_append (&dirs, dir) == -1) {
       nbdkit_error ("realloc: %m");
       free (dir);
       return -1;
     }
-    dirs = new_dirs;
-    dirs[nr_dirs] = dir;
-    nr_dirs++;
   }
   else if (strcmp (key, "params") == 0) {
     params = value;
@@ -170,7 +163,7 @@ iso_config (const char *key, const char *value)
 static int
 iso_config_complete (void)
 {
-  if (nr_dirs == 0) {
+  if (dirs.size == 0) {
     nbdkit_error ("you must supply the dir=<DIRECTORY> parameter
"
                   "after the plugin name on the command line");
     return -1;
diff --git a/plugins/split/split.c b/plugins/split/split.c
index 70fd4d40..a12b1037 100644
--- a/plugins/split/split.c
+++ b/plugins/split/split.c
@@ -47,10 +47,11 @@
 #include <nbdkit-plugin.h>
 
 #include "cleanup.h"
+#include "vector.h"
 
 /* The files. */
-static char **filenames = NULL;
-static size_t nr_files = 0;
+DEFINE_VECTOR_TYPE(string_vector, char *);
+static string_vector filenames = empty_vector;
 
 /* Any callbacks using lseek must be protected by this lock. */
 static pthread_mutex_t lseek_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -58,29 +59,23 @@ static pthread_mutex_t lseek_lock =
PTHREAD_MUTEX_INITIALIZER;
 static void
 split_unload (void)
 {
-  size_t i;
-
-  for (i = 0; i < nr_files; ++i)
-    free (filenames[i]);
-  free (filenames);
+  string_vector_iter (&filenames, (void *) free);
+  free (filenames.ptr);
 }
 
 static int
 split_config (const char *key, const char *value)
 {
-  char **new_filenames;
+  char *s;
 
   if (strcmp (key, "file") == 0) {
-    new_filenames = realloc (filenames, (nr_files+1) * sizeof (char *));
-    if (new_filenames == NULL) {
-      nbdkit_error ("malloc: %m");
+    s = nbdkit_realpath (value);
+    if (s == NULL)
+      return -1;
+    if (string_vector_append (&filenames, s) == -1) {
+      nbdkit_error ("realloc: %m");
       return -1;
     }
-    filenames = new_filenames;
-    filenames[nr_files] = nbdkit_realpath (value);
-    if (filenames[nr_files] == NULL)
-      return -1;
-    nr_files++;
   }
   else {
     nbdkit_error ("unknown parameter '%s'", key);
@@ -122,13 +117,13 @@ split_open (int readonly)
     return NULL;
   }
 
-  h->files = malloc (nr_files * sizeof (struct file));
+  h->files = malloc (filenames.size * sizeof (struct file));
   if (h->files == NULL) {
     nbdkit_error ("malloc: %m");
     free (h);
     return NULL;
   }
-  for (i = 0; i < nr_files; ++i)
+  for (i = 0; i < filenames.size; ++i)
     h->files[i].fd = -1;
 
   /* Open the files. */
@@ -138,34 +133,34 @@ split_open (int readonly)
   else
     flags |= O_RDWR;
 
-  for (i = 0; i < nr_files; ++i) {
-    h->files[i].fd = open (filenames[i], flags);
+  for (i = 0; i < filenames.size; ++i) {
+    h->files[i].fd = open (filenames.ptr[i], flags);
     if (h->files[i].fd == -1) {
-      nbdkit_error ("open: %s: %m", filenames[i]);
+      nbdkit_error ("open: %s: %m", filenames.ptr[i]);
       goto err;
     }
   }
 
   offset = 0;
-  for (i = 0; i < nr_files; ++i) {
+  for (i = 0; i < filenames.size; ++i) {
     h->files[i].offset = offset;
 
     if (fstat (h->files[i].fd, &statbuf) == -1) {
-      nbdkit_error ("stat: %s: %m", filenames[i]);
+      nbdkit_error ("stat: %s: %m", filenames.ptr[i]);
       goto err;
     }
     h->files[i].size = statbuf.st_size;
     offset += statbuf.st_size;
 
     nbdkit_debug ("file[%zu]=%s: offset=%" PRIu64 ",
size=%" PRIu64,
-                  i, filenames[i], h->files[i].offset, h->files[i].size);
+                  i, filenames.ptr[i], h->files[i].offset,
h->files[i].size);
 
 #ifdef SEEK_HOLE
     /* Test if this file supports extents. */
     ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lseek_lock);
     r = lseek (h->files[i].fd, 0, SEEK_DATA);
     if (r == -1 && errno != ENXIO) {
-      nbdkit_debug ("disabling extents: lseek on %s: %m",
filenames[i]);
+      nbdkit_debug ("disabling extents: lseek on %s: %m",
filenames.ptr[i]);
       h->files[i].can_extents = false;
     }
     else
@@ -180,7 +175,7 @@ split_open (int readonly)
   return h;
 
  err:
-  for (i = 0; i < nr_files; ++i) {
+  for (i = 0; i < filenames.size; ++i) {
     if (h->files[i].fd >= 0)
       close (h->files[i].fd);
   }
@@ -196,7 +191,7 @@ split_close (void *handle)
   struct handle *h = handle;
   size_t i;
 
-  for (i = 0; i < nr_files; ++i)
+  for (i = 0; i < filenames.size; ++i)
     close (h->files[i].fd);
   free (h->files);
   free (h);
@@ -243,7 +238,7 @@ static struct file *
 get_file (struct handle *h, uint64_t offset)
 {
   return bsearch (&offset, h->files,
-                  nr_files, sizeof (struct file),
+                  filenames.size, sizeof (struct file),
                   compare_offset);
 }
 
diff --git a/plugins/ssh/ssh.c b/plugins/ssh/ssh.c
index 1fd5f223..ce91ab3f 100644
--- a/plugins/ssh/ssh.c
+++ b/plugins/ssh/ssh.c
@@ -50,6 +50,9 @@
 #include <nbdkit-plugin.h>
 
 #include "minmax.h"
+#include "vector.h"
+
+DEFINE_VECTOR_TYPE(const_string_vector, const char *);
 
 static const char *host = NULL;
 static const char *path = NULL;
@@ -58,8 +61,7 @@ static const char *user = NULL;
 static char *password = NULL;
 static bool verify_remote_host = true;
 static const char *known_hosts = NULL;
-static const char **identity = NULL;
-static size_t nr_identities = 0;
+static const_string_vector identities = empty_vector;
 static uint32_t timeout = 0;
 static bool compression = false;
 
@@ -98,7 +100,7 @@ log_callback (int priority, const char *function, const char
*message, void *vp)
 static void
 ssh_unload (void)
 {
-  free (identity);
+  free (identities.ptr);
   free (password);
 }
 
@@ -133,16 +135,11 @@ ssh_config (const char *key, const char *value)
     known_hosts = value; /* %-expanded, cannot use nbdkit_absolute_path */
 
   else if (strcmp (key, "identity") == 0) {
-    const char **new_identity -      realloc (identity, (nr_identities+1) *
sizeof (const char *));
-    if (new_identity == NULL) {
+    /* %-expanded, cannot use nbdkit_absolute_path on value */
+    if (const_string_vector_append (&identities, value) == -1) {
       nbdkit_error ("realloc: %m");
       return -1;
     }
-    identity = new_identity;
-    /* %-expanded, cannot use nbdkit_absolute_path */
-    identity[nr_identities] = value;
-    nr_identities++;
   }
 
   else if (strcmp (key, "verify-remote-host") == 0) {
@@ -410,11 +407,12 @@ ssh_open (int readonly)
      * as this file is rarely present.
      */
   }
-  for (i = 0; i < nr_identities; ++i) {
-    r = ssh_options_set (h->session, SSH_OPTIONS_ADD_IDENTITY, identity[i]);
+  for (i = 0; i < identities.size; ++i) {
+    r = ssh_options_set (h->session,
+                         SSH_OPTIONS_ADD_IDENTITY, identities.ptr[i]);
     if (r != SSH_OK) {
       nbdkit_error ("failed to add identity in libssh session: %s:
%s",
-                    identity[i], ssh_get_error (h->session));
+                    identities.ptr[i], ssh_get_error (h->session));
       goto err;
     }
   }
-- 
2.25.0

---
 server/extents.c | 49 ++++++++++++++++++------------------------------
 1 file changed, 18 insertions(+), 31 deletions(-)

diff --git a/server/extents.c b/server/extents.c
index 2d609652..4ab5946c 100644
--- a/server/extents.c
+++ b/server/extents.c
@@ -42,6 +42,7 @@
 #include <assert.h>
 
 #include "minmax.h"
+#include "vector.h"
 
 #include "internal.h"
 
@@ -51,9 +52,11 @@
  */
 #define MAX_EXTENTS (1 * 1024 * 1024)
 
+/* Appendable list of extents. */
+DEFINE_VECTOR_TYPE(extents, struct nbdkit_extent);
+
 struct nbdkit_extents {
-  struct nbdkit_extent *extents;
-  size_t nr_extents, allocated;
+  extents extents;
 
   uint64_t start, end; /* end is one byte beyond the end of the range */
 
@@ -92,8 +95,7 @@ nbdkit_extents_new (uint64_t start, uint64_t end)
     nbdkit_error ("nbdkit_extents_new: malloc: %m");
     return NULL;
   }
-  r->extents = NULL;
-  r->nr_extents = r->allocated = 0;
+  r->extents = (extents) empty_vector;
   r->start = start;
   r->end = end;
   r->next = -1;
@@ -104,7 +106,7 @@ void
 nbdkit_extents_free (struct nbdkit_extents *exts)
 {
   if (exts) {
-    free (exts->extents);
+    free (exts->extents.ptr);
     free (exts);
   }
 }
@@ -112,40 +114,25 @@ nbdkit_extents_free (struct nbdkit_extents *exts)
 size_t
 nbdkit_extents_count (const struct nbdkit_extents *exts)
 {
-  return exts->nr_extents;
+  return exts->extents.size;
 }
 
 struct nbdkit_extent
 nbdkit_get_extent (const struct nbdkit_extents *exts, size_t i)
 {
-  assert (i < exts->nr_extents);
-  return exts->extents[i];
+  assert (i < exts->extents.size);
+  return exts->extents.ptr[i];
 }
 
 /* Insert *e in the list at the end. */
 static int
 append_extent (struct nbdkit_extents *exts, const struct nbdkit_extent *e)
 {
-  if (exts->nr_extents >= exts->allocated) {
-    size_t new_allocated;
-    struct nbdkit_extent *new_extents;
-
-    new_allocated = exts->allocated;
-    if (new_allocated == 0)
-      new_allocated = 1;
-    new_allocated *= 2;
-    new_extents -      realloc (exts->extents, new_allocated * sizeof
(struct nbdkit_extent));
-    if (new_extents == NULL) {
-      nbdkit_error ("nbdkit_add_extent: realloc: %m");
-      return -1;
-    }
-    exts->allocated = new_allocated;
-    exts->extents = new_extents;
+  if (extents_append (&exts->extents, *e) == -1) {
+    nbdkit_error ("nbdkit_add_extent: realloc: %m");
+    return -1;
   }
 
-  exts->extents[exts->nr_extents] = *e;
-  exts->nr_extents++;
   return 0;
 }
 
@@ -170,7 +157,7 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
     return 0;
 
   /* Ignore extents beyond the end of the range, or if list is full. */
-  if (offset >= exts->end || exts->nr_extents >= MAX_EXTENTS)
+  if (offset >= exts->end || exts->extents.size >= MAX_EXTENTS)
     return 0;
 
   /* Shorten extents that overlap the end of the range. */
@@ -179,7 +166,7 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
     length -= overlap;
   }
 
-  if (exts->nr_extents == 0) {
+  if (exts->extents.size == 0) {
     /* If there are no existing extents, and the new extent is
      * entirely before start, ignore it.
      */
@@ -206,10 +193,10 @@ nbdkit_add_extent (struct nbdkit_extents *exts,
   }
 
   /* If we get here we are going to either add or extend. */
-  if (exts->nr_extents > 0 &&
-      exts->extents[exts->nr_extents-1].type == type) {
+  if (exts->extents.size > 0 &&
+      exts->extents.ptr[exts->extents.size-1].type == type) {
     /* Coalesce with the last extent. */
-    exts->extents[exts->nr_extents-1].length += length;
+    exts->extents.ptr[exts->extents.size-1].length += length;
     return 0;
   }
   else {
-- 
2.25.0

A fairly straightforward replacement, but note that we must rename all
variables called ‘regions’ as something else (eg. ‘rs’) because
-Wshadow warns about them (which is surprising to me since I thought
this warning only applied to local vs global variable, not local
variable vs global typedef).

Also I got rid of the get_regions accessor method, replacing it
everywhere with direct use of regions->ptr[].  Although the accessor
method did bounds checking, my reasoning is this change is correct
because in all cases we were iterating over the regions from
0 .. nr_regions-1.  However an improvement would be to have a proper
iterator, although that change is not so straightforward because of
lack of closures in C.
---
 common/regions/regions.h             | 37 +++++++++-------------
 common/regions/regions.c             | 46 ++++++++++++----------------
 plugins/partitioning/virtual-disk.h  |  2 +-
 plugins/linuxdisk/partition-gpt.c    |  2 +-
 plugins/partitioning/partition-gpt.c |  8 ++---
 plugins/partitioning/partition-mbr.c | 10 +++---
 plugins/partitioning/partitioning.c  | 12 ++++----
 plugins/partitioning/virtual-disk.c  | 26 ++++++++--------
 8 files changed, 63 insertions(+), 80 deletions(-)

diff --git a/common/regions/regions.h b/common/regions/regions.h
index d6ab5857..dd6b479b 100644
--- a/common/regions/regions.h
+++ b/common/regions/regions.h
@@ -37,6 +37,8 @@
 #include <stdarg.h>
 #include <assert.h>
 
+#include "vector.h"
+
 /* This defines a very simple structure used to define the virtual
  * disk in the partitioning and floppy plugins.
  *
@@ -70,38 +72,35 @@ struct region {
   const char *description;
 };
 
-/* Array of regions. */
-struct regions {
-  struct region *regions;
-  size_t nr_regions;
-};
+/* Vector of struct region. */
+DEFINE_VECTOR_TYPE(regions, struct region);
 
-extern void init_regions (struct regions *regions)
+extern void init_regions (regions *regions)
   __attribute__((__nonnull__ (1)));
-extern void free_regions (struct regions *regions)
+extern void free_regions (regions *regions)
   __attribute__((__nonnull__ (1)));
 
 /* Return the number of regions. */
 static inline size_t __attribute__((__nonnull__ (1)))
-nr_regions (struct regions *regions)
+nr_regions (struct regions *rs)
 {
-  return regions->nr_regions;
+  return rs->size;
 }
 
 /* Return the virtual size of the disk. */
 static inline int64_t __attribute__((__nonnull__ (1)))
-virtual_size (struct regions *regions)
+virtual_size (regions *rs)
 {
-  if (regions->nr_regions == 0)
+  if (rs->size == 0)
     return 0;
   else
-    return regions->regions[regions->nr_regions-1].end + 1;
+    return rs->ptr[rs->size-1].end + 1;
 }
 
 /* Look up the region corresponding to the given offset.  If the
  * offset is inside the disk image then this cannot return NULL.
  */
-extern const struct region *find_region (const struct regions *regions,
+extern const struct region *find_region (const regions *regions,
                                          uint64_t offset)
   __attribute__((__nonnull__ (1)));
 
@@ -119,7 +118,7 @@ extern const struct region *find_region (const struct
regions *regions,
  * If type == region_file, it must be followed by u.i parameter.
  * If type == region_data, it must be followed by u.data parameter.
  */
-extern int append_region_len (struct regions *regions,
+extern int append_region_len (regions *regions,
                               const char *description, uint64_t len,
                               uint64_t pre_aligment, uint64_t post_alignment,
                               enum region_type type, ...);
@@ -129,18 +128,10 @@ extern int append_region_len (struct regions *regions,
  * size of the main region, specify the end byte as an offset.  Note
  * the end byte is included in the region, it's is NOT the end+1 byte.
  */
-extern int append_region_end (struct regions *regions,
+extern int append_region_end (regions *regions,
                               const char *description, uint64_t end,
                               uint64_t pre_aligment, uint64_t post_alignment,
                               enum region_type type, ...);
 #endif
 
-/* Used when iterating over the list of regions. */
-static inline const struct region * __attribute__((__nonnull__ (1)))
-get_region (const struct regions *regions, size_t i)
-{
-  assert (i < regions->nr_regions);
-  return &regions->regions[i];
-}
-
 #endif /* NBDKIT_REGIONS_H */
diff --git a/common/regions/regions.c b/common/regions/regions.c
index ea252df3..d32519b5 100644
--- a/common/regions/regions.c
+++ b/common/regions/regions.c
@@ -43,19 +43,18 @@
 #include "regions.h"
 
 void
-init_regions (struct regions *regions)
+init_regions (regions *rs)
 {
-  regions->regions = NULL;
-  regions->nr_regions = 0;
+  *rs = (regions) empty_vector;
 }
 
 void
-free_regions (struct regions *regions)
+free_regions (struct regions *rs)
 {
   /* We don't need to free the data since that is not owned by the
    * regions structure.
    */
-  free (regions->regions);
+  free (rs->ptr);
 }
 
 /* Find the region corresponding to the given offset.  Use region->end
@@ -73,9 +72,9 @@ compare_offset (const void *offsetp, const void *regionp)
 }
 
 const struct region *
-find_region (const struct regions *regions, uint64_t offset)
+find_region (const regions *rs, uint64_t offset)
 {
-  return bsearch (&offset, regions->regions, regions->nr_regions,
+  return bsearch (&offset, rs->ptr, rs->size,
                   sizeof (struct region), compare_offset);
 }
 
@@ -86,50 +85,43 @@ find_region (const struct regions *regions, uint64_t offset)
  * construct regions out of order using this function.
  */
 static int __attribute__((__nonnull__ (1)))
-append_one_region (struct regions *regions, struct region region)
+append_one_region (regions *rs, struct region region)
 {
-  struct region *p;
-
   /* The assertions in this function are meant to maintain the
    * invariant about the array as described at the top of this file.
    */
-  assert (region.start == virtual_size (regions));
+  assert (region.start == virtual_size (rs));
   assert (region.len > 0);
   assert (region.end >= region.start);
   assert (region.len == region.end - region.start + 1);
 
-  p = realloc (regions->regions,
-               (regions->nr_regions+1) * sizeof (struct region));
-  if (p == NULL) {
+  if (regions_append (rs, region) == -1) {
     nbdkit_error ("realloc: %m");
     return -1;
   }
-  regions->regions = p;
-  regions->regions[regions->nr_regions] = region;
-  regions->nr_regions++;
 
   return 0;
 }
 
 static int
-append_padding (struct regions *regions, uint64_t alignment)
+append_padding (regions *rs, uint64_t alignment)
 {
   struct region region;
 
   assert (is_power_of_2 (alignment));
 
-  region.start = virtual_size (regions);
+  region.start = virtual_size (rs);
   if (IS_ALIGNED (region.start, alignment))
     return 0;                   /* nothing to do */
   region.end = (region.start & ~(alignment-1)) + alignment - 1;
   region.len = region.end - region.start + 1;
   region.type = region_zero;
   region.description = "padding";
-  return append_one_region (regions, region);
+  return append_one_region (rs, region);
 }
 
 int
-append_region_len (struct regions *regions,
+append_region_len (regions *rs,
                    const char *description, uint64_t len,
                    uint64_t pre_aligment, uint64_t post_alignment,
                    enum region_type type, ...)
@@ -138,14 +130,14 @@ append_region_len (struct regions *regions,
 
   /* Pre-alignment. */
   if (pre_aligment != 0) {
-    if (append_padding (regions, pre_aligment) == -1)
+    if (append_padding (rs, pre_aligment) == -1)
       return -1;
-    assert (IS_ALIGNED (virtual_size (regions), pre_aligment));
+    assert (IS_ALIGNED (virtual_size (rs), pre_aligment));
   }
 
   /* Main region. */
   region.description = description;
-  region.start = virtual_size (regions);
+  region.start = virtual_size (rs);
   region.len = len;
   region.end = region.start + region.len - 1;
   region.type = type;
@@ -167,14 +159,14 @@ append_region_len (struct regions *regions,
     va_end (ap);
     region.u.data = data;
   }
-  if (append_one_region (regions, region) == -1)
+  if (append_one_region (rs, region) == -1)
     return -1;
 
   /* Post-alignment. */
   if (post_alignment != 0) {
-    if (append_padding (regions, post_alignment) == -1)
+    if (append_padding (rs, post_alignment) == -1)
       return -1;
-    assert (IS_ALIGNED (virtual_size (regions), post_alignment));
+    assert (IS_ALIGNED (virtual_size (rs), post_alignment));
   }
 
   return 0;
diff --git a/plugins/partitioning/virtual-disk.h
b/plugins/partitioning/virtual-disk.h
index e1739783..4428ff17 100644
--- a/plugins/partitioning/virtual-disk.h
+++ b/plugins/partitioning/virtual-disk.h
@@ -91,7 +91,7 @@ struct file {
 extern struct file *files;
 extern size_t nr_files;
 
-extern struct regions regions;
+extern regions the_regions;
 extern unsigned char *primary, *secondary, **ebr;
 
 /* Main entry point called after files array has been populated. */
diff --git a/plugins/linuxdisk/partition-gpt.c
b/plugins/linuxdisk/partition-gpt.c
index 81a530d6..6380dc8f 100644
--- a/plugins/linuxdisk/partition-gpt.c
+++ b/plugins/linuxdisk/partition-gpt.c
@@ -197,7 +197,7 @@ create_gpt_partition_table (struct virtual_disk *disk,
unsigned char *out)
   size_t j;
 
   for (j = 0; j < nr_regions (&disk->regions); ++j) {
-    const struct region *region = get_region (&disk->regions, j);
+    const struct region *region = &disk->regions.ptr[j];
 
     /* Find the (only) partition region, which has type region_file. */
     if (region->type == region_file) {
diff --git a/plugins/partitioning/partition-gpt.c
b/plugins/partitioning/partition-gpt.c
index 75b4643a..b2843414 100644
--- a/plugins/partitioning/partition-gpt.c
+++ b/plugins/partitioning/partition-gpt.c
@@ -88,7 +88,7 @@ create_gpt_partition_header (const void *pt, bool is_primary,
   uint64_t nr_lbas;
   struct gpt_header *header = (struct gpt_header *) out;
 
-  nr_lbas = virtual_size (&regions) / SECTOR_SIZE;
+  nr_lbas = virtual_size (&the_regions) / SECTOR_SIZE;
 
   memset (header, 0, sizeof *header);
   memcpy (header->signature, GPT_SIGNATURE, sizeof (header->signature));
@@ -122,8 +122,8 @@ create_gpt_partition_table (unsigned char *out)
 {
   size_t i, j;
 
-  for (j = 0; j < nr_regions (&regions); ++j) {
-    const struct region *region = get_region (&regions, j);
+  for (j = 0; j < nr_regions (&the_regions); ++j) {
+    const struct region *region = &the_regions.ptr[j];
 
     if (region->type == region_file) {
       i = region->u.i;
@@ -188,7 +188,7 @@ create_gpt_protective_mbr (unsigned char *out)
    * expressible with MBR.
    */
   region.start = 512;
-  end = virtual_size (&regions) - 1;
+  end = virtual_size (&the_regions) - 1;
   if (end > UINT32_MAX * SECTOR_SIZE)
     end = UINT32_MAX * SECTOR_SIZE;
   region.end = end;
diff --git a/plugins/partitioning/partition-mbr.c
b/plugins/partitioning/partition-mbr.c
index 971e694f..5c18966c 100644
--- a/plugins/partitioning/partition-mbr.c
+++ b/plugins/partitioning/partition-mbr.c
@@ -90,7 +90,7 @@ create_mbr_layout (void)
      */
     eptr0 = find_ebr_region (3, &j);
     region.start = eptr0->start;
-    region.end = virtual_size (&regions) - 1; /* to end of disk */
+    region.end = virtual_size (&the_regions) - 1; /* to end of disk */
     region.len = region.end - region.start + 1;
     create_mbr_partition_table_entry (&region, false, 0xf,
&primary[0x1ee]);
 
@@ -144,8 +144,8 @@ find_file_region (size_t i, size_t *j)
 {
   const struct region *region;
 
-  for (; *j < nr_regions (&regions); ++(*j)) {
-    region = get_region (&regions, *j);
+  for (; *j < nr_regions (&the_regions); ++(*j)) {
+    region = &the_regions.ptr[*j];
     if (region->type == region_file && region->u.i == i)
       return region;
   }
@@ -162,8 +162,8 @@ find_ebr_region (size_t i, size_t *j)
 
   assert (i >= 3);
 
-  for (; *j < nr_regions (&regions); ++(*j)) {
-    region = get_region (&regions, *j);
+  for (; *j < nr_regions (&the_regions); ++(*j)) {
+    region = &the_regions.ptr[*j];
     if (region->type == region_data && region->u.data ==
ebr[i-3])
       return region;
   }
diff --git a/plugins/partitioning/partitioning.c
b/plugins/partitioning/partitioning.c
index 865acd28..33379569 100644
--- a/plugins/partitioning/partitioning.c
+++ b/plugins/partitioning/partitioning.c
@@ -79,7 +79,7 @@ struct file *files = NULL;
 size_t nr_files = 0;
 
 /* Virtual disk layout. */
-struct regions regions;
+regions the_regions;
 
 /* Primary and secondary partition tables and extended boot records.
  * Secondary PT is only used for GPT.  EBR array of sectors is only
@@ -93,7 +93,7 @@ static struct random_state random_state;
 static void
 partitioning_load (void)
 {
-  init_regions (&regions);
+  init_regions (&the_regions);
   parse_guid (DEFAULT_TYPE_GUID, type_guid);
   xsrandom (time (NULL), &random_state);
 }
@@ -110,7 +110,7 @@ partitioning_unload (void)
   /* We don't need to free regions.regions[].u.data because it points
    * to primary, secondary or ebr which we free here.
    */
-  free_regions (&regions);
+  free_regions (&the_regions);
 
   free (primary);
   free (secondary);
@@ -295,7 +295,7 @@ partitioning_open (int readonly)
 static int64_t
 partitioning_get_size (void *handle)
 {
-  return virtual_size (&regions);
+  return virtual_size (&the_regions);
 }
 
 /* Serves the same data over multiple connections. */
@@ -318,7 +318,7 @@ static int
 partitioning_pread (void *handle, void *buf, uint32_t count, uint64_t offset)
 {
   while (count > 0) {
-    const struct region *region = find_region (&regions, offset);
+    const struct region *region = find_region (&the_regions, offset);
     size_t i, len;
     ssize_t r;
 
@@ -366,7 +366,7 @@ partitioning_pwrite (void *handle,
                      const void *buf, uint32_t count, uint64_t offset)
 {
   while (count > 0) {
-    const struct region *region = find_region (&regions, offset);
+    const struct region *region = find_region (&the_regions, offset);
     size_t i, len;
     ssize_t r;
 
diff --git a/plugins/partitioning/virtual-disk.c
b/plugins/partitioning/virtual-disk.c
index bb97d007..5a16fb04 100644
--- a/plugins/partitioning/virtual-disk.c
+++ b/plugins/partitioning/virtual-disk.c
@@ -55,7 +55,7 @@ create_virtual_disk_layout (void)
 {
   size_t i;
 
-  assert (nr_regions (&regions) == 0);
+  assert (nr_regions (&the_regions) == 0);
   assert (nr_files > 0);
   assert (primary == NULL);
   assert (secondary == NULL);
@@ -104,13 +104,13 @@ create_virtual_disk_layout (void)
 
   /* Virtual primary partition table region at the start of the disk. */
   if (parttype == PARTTYPE_MBR) {
-    if (append_region_len (&regions, "MBR",
+    if (append_region_len (&the_regions, "MBR",
                            SECTOR_SIZE, 0, 0,
                            region_data, primary) == -1)
       return -1;
   }
   else /* PARTTYPE_GPT */ {
-    if (append_region_len (&regions, "GPT primary",
+    if (append_region_len (&the_regions, "GPT primary",
                            (2+GPT_PTA_LBAs) * SECTOR_SIZE, 0, 0,
                            region_data, primary) == -1)
       return -1;
@@ -120,7 +120,7 @@ create_virtual_disk_layout (void)
   for (i = 0; i < nr_files; ++i) {
     uint64_t offset;
 
-    offset = virtual_size (&regions);
+    offset = virtual_size (&the_regions);
     /* Because we add padding after each partition, this invariant
      * must always be true.
      */
@@ -128,7 +128,7 @@ create_virtual_disk_layout (void)
 
     /* Logical partitions are preceeded by an EBR. */
     if (parttype == PARTTYPE_MBR && nr_files > 4 && i >=
3) {
-      if (append_region_len (&regions, "EBR",
+      if (append_region_len (&the_regions, "EBR",
                              SECTOR_SIZE, 0, 0,
                              region_data, ebr[i-3]) == -1)
         return -1;
@@ -139,7 +139,7 @@ create_virtual_disk_layout (void)
      * If the file size is not a multiple of SECTOR_SIZE then
      * add a padding region at the end to round it up.
      */
-    if (append_region_len (&regions, files[i].filename,
+    if (append_region_len (&the_regions, files[i].filename,
                            files[i].statbuf.st_size,
                            files[i].alignment, SECTOR_SIZE,
                            region_file, i) == -1)
@@ -148,15 +148,15 @@ create_virtual_disk_layout (void)
 
   /* For GPT add the virtual secondary/backup partition table. */
   if (parttype == PARTTYPE_GPT) {
-    if (append_region_len (&regions, "GPT secondary",
+    if (append_region_len (&the_regions, "GPT secondary",
                            (GPT_PTA_LBAs+1) * SECTOR_SIZE, 0, 0,
                            region_data, secondary) == -1)
       return -1;
   }
 
   if (partitioning_debug_regions) {
-    for (i = 0; i < nr_regions (&regions); ++i) {
-      const struct region *region = get_region (&regions, i);
+    for (i = 0; i < nr_regions (&the_regions); ++i) {
+      const struct region *region = &the_regions.ptr[i];
 
       nbdkit_debug ("region[%zu]: %" PRIx64 "-%" PRIx64
" type=%s",
                     i, region->start, region->end,
@@ -168,13 +168,13 @@ create_virtual_disk_layout (void)
   }
 
   /* We must have created some regions. */
-  assert (nr_regions (&regions) > 0);
+  assert (nr_regions (&the_regions) > 0);
 
   /* Check the final alignment of all the partitions is the same as
    * what was requested.
    */
-  for (i = 0; i < nr_regions (&regions); ++i) {
-    const struct region *region = get_region (&regions, i);
+  for (i = 0; i < nr_regions (&the_regions); ++i) {
+    const struct region *region = &the_regions.ptr[i];
 
     if (region->type == region_file)
       assert (IS_ALIGNED (region->start, files[region->u.i].alignment));
@@ -189,7 +189,7 @@ create_partition_table (void)
   /* The caller has already created the disk layout and allocated
    * space in memory for the partition table.
    */
-  assert (nr_regions (&regions) > 0);
+  assert (nr_regions (&the_regions) > 0);
   assert (primary != NULL);
   if (parttype == PARTTYPE_GPT)
     assert (secondary != NULL);
-- 
2.25.0

---
 TODO | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/TODO b/TODO
index 25b48498..c4a1625b 100644
--- a/TODO
+++ b/TODO
@@ -98,6 +98,14 @@ General ideas for improvements
 
 * Examine other fuzzers: https://gitlab.com/akihe/radamsa
 
+* common/include/vector.h could be extended and used in other places:
+  - there are some more possible places in the server (anywhere using
+    realloc is suspect)
+  - nbdkit-vddk-plugin, where it builds the new argv
+  - allow insertion, and use it in nbdkit-extentlist-filter
+  - allow insertion and keep sorted, use it in nbdkit-eval-plugin
+  - same, and use it in common/sparse/sparse.c
+
 Suggestions for plugins
 -----------------------
 
-- 
2.25.0

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(+)

diff --git a/common/utils/Makefile.am b/common/utils/Makefile.am
index 9a78339d..41761c79 100644
--- a/common/utils/Makefile.am
+++ b/common/utils/Makefile.am
@@ -37,12 +37,14 @@ libutils_la_SOURCES = \
 	cleanup.c \
 	cleanup-nbdkit.c \
 	cleanup.h \
+	environ.c \
 	quote.c \
 	utils.c \
 	utils.h \
 	$(NULL)
 libutils_la_CPPFLAGS = \
 	-I$(top_srcdir)/include \
+	-I$(top_srcdir)/common/include \
 	$(NULL)
 libutils_la_CFLAGS = $(WARNINGS_CFLAGS)
 
diff --git a/common/utils/utils.h b/common/utils/utils.h
index ebd5f66b..5c121ccb 100644
--- a/common/utils/utils.h
+++ 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, ...);
 
 #endif /* NBDKIT_UTILS_H */
diff --git a/common/utils/environ.c b/common/utils/environ.c
new file mode 100644
index 00000000..a43eeb71
--- /dev/null
+++ b/common/utils/environ.c
@@ -0,0 +1,116 @@
+/* nbdkit
+ * Copyright (C) 2018-2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <string.h>
+
+#include <nbdkit-plugin.h>
+
+#include "utils.h"
+#include "vector.h"
+
+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) ... */
+    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: ;
+  }
+  va_end (argp);
+
+  /* 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;
+}
-- 
2.25.0

This automatically frees an argv-style list of strings, such as a copy
of environ.
---
 common/utils/cleanup.h |  3 +++
 common/utils/cleanup.c | 13 +++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/common/utils/cleanup.h b/common/utils/cleanup.h
index 7070290d..14ffc7ff 100644
--- a/common/utils/cleanup.h
+++ b/common/utils/cleanup.h
@@ -40,6 +40,9 @@
 extern void cleanup_free (void *ptr);
 #define CLEANUP_FREE __attribute__((cleanup (cleanup_free)))
 
+extern void cleanup_free_string_list (char ***ptr);
+#define CLEANUP_FREE_STRING_LIST __attribute__((cleanup
(cleanup_free_string_list)))
+
 extern void cleanup_unlock (pthread_mutex_t **ptr);
 #define CLEANUP_UNLOCK __attribute__((cleanup (cleanup_unlock)))
 
diff --git a/common/utils/cleanup.c b/common/utils/cleanup.c
index fb77805b..a9759ec4 100644
--- a/common/utils/cleanup.c
+++ b/common/utils/cleanup.c
@@ -44,6 +44,19 @@ cleanup_free (void *ptr)
   free (* (void **) ptr);
 }
 
+void
+cleanup_free_string_list (char ***ptr)
+{
+  size_t i;
+  char **argv = *ptr;
+
+  if (argv == NULL) return;
+
+  for (i = 0; argv[i] != NULL; ++i)
+    free (argv[i]);
+  free (argv);
+}
+
 void
 cleanup_unlock (pthread_mutex_t **ptr)
 {
-- 
2.25.0

The $tmpdir environment variable is used by the eval and sh plugins to
communicate the path to the temporary directory created by the plugin
for shell scripts to use for temporary files.

Previously we set this environment variable globally (in .load()), but
this means the environment variable can be leaked into undesirable
places, eg. into the --run script:

  $ nbdkit sh - --run 'echo tmpdir: $tmpdir' </dev/null
  tmpdir: /tmp/nbdkitshco6MC9

By setting it only just before running the shell command it should
only be visible there.  Note also we no longer use setenv(3), but
instead we manipulate environ.

After this change:

  $ ./nbdkit sh - --run 'echo tmpdir: $tmpdir' </dev/null
  tmpdir:
---
 plugins/sh/call.h   |  2 ++
 plugins/eval/eval.c |  7 +------
 plugins/sh/call.c   | 11 +++++++++++
 plugins/sh/sh.c     |  7 +------
 4 files changed, 15 insertions(+), 12 deletions(-)

diff --git a/plugins/sh/call.h b/plugins/sh/call.h
index 6aa70e56..78305d1e 100644
--- a/plugins/sh/call.h
+++ b/plugins/sh/call.h
@@ -49,4 +49,6 @@ extern exit_code call_write (const char *wbuf, size_t wbuflen,
                              const char **argv)
   __attribute__((__nonnull__ (1, 3)));
 
+extern char tmpdir[];
+
 #endif /* NBDKIT_CALL_H */
diff --git a/plugins/eval/eval.c b/plugins/eval/eval.c
index f09e49f3..3bc7f2a6 100644
--- a/plugins/eval/eval.c
+++ b/plugins/eval/eval.c
@@ -50,7 +50,7 @@
 #include "call.h"
 #include "methods.h"
 
-static char tmpdir[] = "/tmp/nbdkitevalXXXXXX";
+char tmpdir[] = "/tmp/nbdkitevalXXXXXX";
 static char *missing;
 
 static const char *known_methods[] = {
@@ -218,11 +218,6 @@ eval_load (void)
     nbdkit_error ("mkdtemp: /tmp: %m");
     exit (EXIT_FAILURE);
   }
-  /* Set $tmpdir for the script. */
-  if (setenv ("tmpdir", tmpdir, 1) == -1) {
-    nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
-    exit (EXIT_FAILURE);
-  }
 
   nbdkit_debug ("eval: load: tmpdir: %s", tmpdir);
 
diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index 2d99a120..b0aaf754 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -106,6 +106,7 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
        const char **argv)                /* script + parameters */
 {
   const char *argv0 = argv[0]; /* script name, used in error messages */
+  CLEANUP_FREE_STRING_LIST char **env = NULL;
   pid_t pid = -1;
   int status;
   int ret = ERROR;
@@ -122,6 +123,11 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin
*/
 
   debug_call (argv);
 
+  /* Copy the environment, and add $tmpdir. */
+  env = copy_environ (environ, "tmpdir", tmpdir, NULL);
+  if (env == NULL)
+    goto error;
+
 #ifdef HAVE_PIPE2
   if (pipe2 (in_fd, O_CLOEXEC) == -1) {
     nbdkit_error ("%s: pipe2: %m", argv0);
@@ -184,6 +190,11 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin
*/
     /* Restore SIGPIPE back to SIG_DFL, since shell can't undo SIG_IGN */
     signal (SIGPIPE, SIG_DFL);
 
+    /* Note the assignment of environ avoids using execvpe which is a
+     * GNU extension.  See also:
+     *
https://github.com/libguestfs/libnbd/commit/dc64ac5cdd0bc80ca4e18935ad0e8801d11a8644
+     */
+    environ = env;
     execvp (argv[0], (char **) argv);
     perror (argv[0]);
     _exit (EXIT_FAILURE);
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index c8a321f1..deb01201 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -50,7 +50,7 @@
 #include "call.h"
 #include "methods.h"
 
-static char tmpdir[] = "/tmp/nbdkitshXXXXXX";
+char tmpdir[] = "/tmp/nbdkitshXXXXXX";
 static char *script;
 static char *magic_config_key;
 
@@ -71,11 +71,6 @@ sh_load (void)
     nbdkit_error ("mkdtemp: /tmp: %m");
     exit (EXIT_FAILURE);
   }
-  /* Set $tmpdir for the script. */
-  if (setenv ("tmpdir", tmpdir, 1) == -1) {
-    nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
-    exit (EXIT_FAILURE);
-  }
 
   nbdkit_debug ("sh: load: tmpdir: %s", tmpdir);
 }
-- 
2.25.0

---
 plugins/eval/nbdkit-eval-plugin.pod | 11 +++--------
 plugins/sh/nbdkit-sh-plugin.pod     | 18 +++++++++++++++++-
 plugins/sh/call.c                   |  8 ++++++--
 tests/test-single-sh.sh             |  4 ++++
 4 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/plugins/eval/nbdkit-eval-plugin.pod
b/plugins/eval/nbdkit-eval-plugin.pod
index 602fd3aa..a016aa78 100644
--- a/plugins/eval/nbdkit-eval-plugin.pod
+++ b/plugins/eval/nbdkit-eval-plugin.pod
@@ -154,14 +154,9 @@ defaults to the script "exit 2".
 
 =head1 ENVIRONMENT VARIABLES
 
-=over 4
-
-=item C<tmpdir>
-
-This is defined to the name of a temporary directory which can be used
-by the script snippets.  It is deleted when nbdkit exits.
-
-=back
+For environment variables that are set by the plugin running the
+script snippets, see
+L<nbdkit-sh-plugin(3)/Environment variables passed to the script>.
 
 =head1 FILES
 
diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index 20a2b785..33bae6e4 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -119,7 +119,14 @@ These exit codes are reserved for future use.
 
 =back
 
-=head2 Temporary directory
+=head2 Environment variables passed to the script
+
+When running the script, certain environment variables are set by the
+plugin:
+
+=over 4
+
+=item C<$tmpdir>
 
 A fresh script is invoked for each method call (ie. scripts are
 stateless), so if the script needs to store state it has to store it
@@ -131,6 +138,15 @@ the script.  This directory persists for the lifetime of
nbdkit and is
 deleted when nbdkit exits.  The name of the directory is passed to
 each script invocation in the C<$tmpdir> environment variable.
 
+=item C<$nbdkit_stdio_safe>
+
+This is set to C<1> or C<0> depending on whether or not the script
can
+safely use stdin/stdout (eg. for reading passwords from the user).
+See also the discussion of C<nbdkit_stdio_safe> in
+L<nbdkit-plugin(3)>.
+
+=back
+
 =head2 Handles
 
 Handles are arbitrary strings, but it is best to limit them to short
diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index b0aaf754..ddb7045b 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -107,6 +107,7 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
 {
   const char *argv0 = argv[0]; /* script name, used in error messages */
   CLEANUP_FREE_STRING_LIST char **env = NULL;
+  const char *stdio_safe_str = nbdkit_stdio_safe () ? "1" :
"0";
   pid_t pid = -1;
   int status;
   int ret = ERROR;
@@ -123,8 +124,11 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin
*/
 
   debug_call (argv);
 
-  /* Copy the environment, and add $tmpdir. */
-  env = copy_environ (environ, "tmpdir", tmpdir, NULL);
+  /* Copy the environment, and add $tmpdir and some other variables. */
+  env = copy_environ (environ,
+                      "tmpdir", tmpdir,
+                      "nbdkit_stdio_safe", stdio_safe_str,
+                      NULL);
   if (env == NULL)
     goto error;
 
diff --git a/tests/test-single-sh.sh b/tests/test-single-sh.sh
index a1586868..b4cfc9f0 100755
--- a/tests/test-single-sh.sh
+++ b/tests/test-single-sh.sh
@@ -57,6 +57,10 @@ if test -f single-sh.log; then
 fi
 
 cat >single-sh.script <<\EOF
+if test "$nbdkit_stdio_safe" != "0"; then
+  echo "unexpected value for nbdkit_stdio_safe ($nbdkit_stdio_safe)"
1>&2
+  exit 1
+fi
 case $1 in
   get_size) echo 1m ;;
   pread) dd if=/dev/zero count=$3 iflag=count_bytes ;;
-- 
2.25.0

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:

In the subject, you describe $nbdkit_safe_stdio, but in the patch body...
> ---
>   plugins/eval/nbdkit-eval-plugin.pod | 11 +++--------
>   plugins/sh/nbdkit-sh-plugin.pod     | 18 +++++++++++++++++-
>   plugins/sh/call.c                   |  8 ++++++--
>   tests/test-single-sh.sh             |  4 ++++
>   4 files changed, 30 insertions(+), 11 deletions(-)
> 
The patch does not work as intended as currently written: when we invoke 
/path/to/script config $key $value, we have already set up stdin/stdout 
to our own pipes [1].  For .config and .config_complete, reading will 
always see EOF (no other callback needs to interact with the original 
stdin, and callbacks like .pwrite actually use the pipe for data).  If 
we want to allow a script to read a password from stdin, we need to 
preserve the original fd to .config and .config_complete rather than 
passing in an empty pipe (but those are the only two callbacks where it 
makes sense, and even then, only when we did not use script=- or when -s 
is in effect).

[1] ./nbdkit eval config='ls -l /proc/$$/fd >/dev/tty' a=b
total 0
lr-x------. 1 eblake eblake 64 Apr 15 13:03 0 -> 'pipe:[1669308]'
l-wx------. 1 eblake eblake 64 Apr 15 13:03 1 -> 'pipe:[1669309]'
l-wx------. 1 eblake eblake 64 Apr 15 13:03 2 -> 'pipe:[1669310]'
lr-x------. 1 eblake eblake 64 Apr 15 13:03 255 -> 
/tmp/nbdkitevalVUNZ1F/config
> +++ b/plugins/sh/nbdkit-sh-plugin.pod
> @@ -119,7 +119,14 @@ These exit codes are reserved for future use.
>   
>   =back
>   
> -=head2 Temporary directory
> +=head2 Environment variables passed to the script
> +
> +When running the script, certain environment variables are set by the
> +plugin:
> +
> +=over 4
> +
> +=item C<$tmpdir>
>   
>   A fresh script is invoked for each method call (ie. scripts are
>   stateless), so if the script needs to store state it has to store it
> @@ -131,6 +138,15 @@ the script.  This directory persists for the lifetime
of nbdkit and is
>   deleted when nbdkit exits.  The name of the directory is passed to
>   each script invocation in the C<$tmpdir> environment variable.
>   
> +=item C<$nbdkit_stdio_safe>
...this name makes more sense (matching the public API), but disagrees 
with the commit title.

Side thought: Both the eval and sh plugins already pass on all 
unrecognized key=value pairs through to the .config callback, and error 
out if the config callback returns missing.  But right now, if a script 
wants to set up an environment variable that will still be present to 
affect later calls, it has to track things itself (perhaps by having the 
.config callback append to $tmpdir/vars, then all other callbacks start 
by 'source $tmpdir/vars').  Would it make sense to reserve a special 
exit value from the .config callback for the case where the script wants 
the current key=value passed to config to be preserved to all future 
callbacks?  Or even state that the config callback exiting with status 2 
(for missing) is NOT an error, but does the key=value preservation 
automatically?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> Can be used for building up lists of things, especially
> lists of strings.
> ---
>   common/include/Makefile.am   |  6 +++
>   common/include/test-vector.c | 90 +++++++++++++++++++++++++++++++++
>   common/include/vector.h      | 96 ++++++++++++++++++++++++++++++++++++
>   .gitignore                   |  1 +
>   4 files changed, 193 insertions(+)
> 
> +++ b/common/include/vector.h
> +/* Simple implementation of appendable vector.  There are two main
> + * use-cases we consider: lists of strings (either with a defined
> + * length, or NULL-terminated), and lists of numbers.  It is generic
> + * so could be used for lists of anything (eg. structs) where being
> + * able to append easily is important.
> + */
> +
> +#ifndef NBDKIT_VECTOR_H
> +#define NBDKIT_VECTOR_H
> +
> +#include <assert.h>
> +
> +#define DEFINE_VECTOR_TYPE(name, type)                                  \
> +  struct name {                                                         \
> +    type *ptr;                 /* Pointer to array of items. */         \
> +    size_t size;               /* Number of valid items in the array. */ \
> +    size_t alloc;              /* Number of items allocated. */         \
> +  };                                                                    \
> +  typedef struct name name;                                             \
> +  static inline int                                                     \
> +  name##_extend (name *v, size_t n)                                     \
> +  {                                                                     \
> +    return generic_vector_extend ((struct generic_vector *)v, n,        \
> +                                  sizeof (type));                       \
> +  }                                                                     \
> +  static inline int                                                     \
> +  name##_append (name *v, type elem)                                    \
> +  {                                                                     \
> +    if (v->size >= v->alloc) {                                   
\
> +      if (name##_extend (v, 1) == -1) return -1;                        \
> +    }                                                                   \
> +    v->ptr[v->size++] = elem;                                       
\
> +    return 0;                                                           \
> +  }                                                                     \
> +  static inline void                                                    \
> +  name##_iter (name *v, void (*f) (type elem))                          \
Do we want an iterator that can take a void* opaque argument, as in:

name##_iter_arg (name *v, void (*f) (type elem, void *o), void *o)

Of course, since you want to pass 'free' to string_vector, you'll
still
want the form without an opaque argument as well.
> +  {                                                                     \
> +    size_t i;                                                           \
> +    for (i = 0; i < v->size; ++i)                                   
\
> +      f (v->ptr[i]);                                                   
\
> +  }
Should we allow the callback to return a value, where returning 0 can 
abort the iteration early?
> +
> +#define empty_vector { .ptr = NULL, .size = 0, .alloc = 0 }
Nice that this initializer is type-agnostic.
> +
> +struct generic_vector {
> +  void *ptr;
> +  size_t size;
> +  size_t alloc;
> +};
> +
> +static int
> +generic_vector_extend (struct generic_vector *v, size_t n, size_t
itemsize)
> +{
> +  void *newptr;
> +
> +  newptr = realloc (v->ptr, (n + v->alloc) * itemsize);
> +  if (newptr == NULL)
> +    return -1;
> +  v->ptr = newptr;
> +  v->alloc += n;
> +  return 0;
> +}
Do we really want this implemented in the header?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> These plugins have in common that they store either a list of
> allocated strings or a list of constant strings.  Define either
> string_vector or const_string_vector as appropriate and use it to
> store these lists.
> ---
The commit message mentions strings...

> +++ b/plugins/floppy/virtual-floppy.h
> @@ -37,6 +37,7 @@
>   #include <sys/stat.h>
>   
>   #include "regions.h"
> +#include "vector.h"
>   
>   struct partition_entry {
>     uint8_t bootable;             /* 0x00 or 0x80 if bootable */
> @@ -130,6 +131,9 @@ struct dir_entry {
>     uint32_t size;                /* 0x1C - file size */
>   } __attribute__((packed));
>   
> +/* Appendable list of struct dir_entry. */
> +DEFINE_VECTOR_TYPE(dir_entries, struct dir_entry);
> +
...but the very first usage is a struct.

The code changes look reasonable.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> ---
>   server/extents.c | 49 ++++++++++++++++++------------------------------
>   1 file changed, 18 insertions(+), 31 deletions(-)
> 
ACK.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> A fairly straightforward replacement, but note that we must rename all
> variables called ‘regions’ as something else (eg. ‘rs’) because
> -Wshadow warns about them (which is surprising to me since I thought
> this warning only applied to local vs global variable, not local
> variable vs global typedef).
> 
> Also I got rid of the get_regions accessor method, replacing it
> everywhere with direct use of regions->ptr[].  Although the accessor
> method did bounds checking, my reasoning is this change is correct
> because in all cases we were iterating over the regions from
> 0 .. nr_regions-1.  However an improvement would be to have a proper
> iterator, although that change is not so straightforward because of
> lack of closures in C.
> ---
> @@ -70,38 +72,35 @@ struct region {
>     const char *description;
>   };
>   
> -/* Array of regions. */
> -struct regions {
> -  struct region *regions;
> -  size_t nr_regions;
> -};
> +/* Vector of struct region. */
> +DEFINE_VECTOR_TYPE(regions, struct region);
>   
> -extern void init_regions (struct regions *regions)
> +extern void init_regions (regions *regions)
>     __attribute__((__nonnull__ (1)));
This change makes sense (DEFINE_VECTOR_TYPE gives us a typedef, so you 
lose the explicit 'struct').
> -extern void free_regions (struct regions *regions)
> +extern void free_regions (regions *regions)
>     __attribute__((__nonnull__ (1)));
>   
>   /* Return the number of regions. */
>   static inline size_t __attribute__((__nonnull__ (1)))
> -nr_regions (struct regions *regions)
> +nr_regions (struct regions *rs)
whereas this one is odd.  Did you mean to drop 'struct' here?  If so, do
you still have to rename the variable to rs?  Okay, I'm seeing the 
pattern - forward declarations don't trigger -Wshadow, but 
implementations do.
>   {
> -  return regions->nr_regions;
> +  return rs->size;
>   }
>   
>   /* Return the virtual size of the disk. */
>   static inline int64_t __attribute__((__nonnull__ (1)))
> -virtual_size (struct regions *regions)
> +virtual_size (regions *rs)
here, you both dropped 'struct' and did the rename; the rename because 
it is the implementation.
> +++ b/common/regions/regions.c
Overall, the rest of the patch is reasonable (mostly mechanical due to 
the renames).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> ---
>   TODO | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/TODO b/TODO
> index 25b48498..c4a1625b 100644
> --- a/TODO
> +++ b/TODO
> @@ -98,6 +98,14 @@ General ideas for improvements
>   
>   * Examine other fuzzers: https://gitlab.com/akihe/radamsa
>   
> +* common/include/vector.h could be extended and used in other places:
> +  - there are some more possible places in the server (anywhere using
> +    realloc is suspect)
> +  - nbdkit-vddk-plugin, where it builds the new argv
> +  - allow insertion, and use it in nbdkit-extentlist-filter
> +  - allow insertion and keep sorted, use it in nbdkit-eval-plugin
> +  - same, and use it in common/sparse/sparse.c
ACK, along with any other ideas we've brought up (such as an iterator 
allowing short-circuiting by returning a value, or ...). Or if you 
actually write some of those patches before committing the TODO change...

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> This automatically frees an argv-style list of strings, such as a copy
> of environ.
> ---
>   common/utils/cleanup.h |  3 +++
>   common/utils/cleanup.c | 13 +++++++++++++
>   2 files changed, 16 insertions(+)
> 
ACK

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

On 4/15/20 11:16 AM, Richard W.M. Jones wrote:
> The $tmpdir environment variable is used by the eval and sh plugins to
> communicate the path to the temporary directory created by the plugin
> for shell scripts to use for temporary files.
> 
> Previously we set this environment variable globally (in .load()), but
> this means the environment variable can be leaked into undesirable
> places, eg. into the --run script:
> 
>    $ nbdkit sh - --run 'echo tmpdir: $tmpdir' </dev/null
>    tmpdir: /tmp/nbdkitshco6MC9
Good catch.
> 
> By setting it only just before running the shell command it should
> only be visible there.  Note also we no longer use setenv(3), but
> instead we manipulate environ.
> 
> After this change:
> 
>    $ ./nbdkit sh - --run 'echo tmpdir: $tmpdir' </dev/null
>    tmpdir:
> ---
>   plugins/sh/call.h   |  2 ++
>   plugins/eval/eval.c |  7 +------
>   plugins/sh/call.c   | 11 +++++++++++
>   plugins/sh/sh.c     |  7 +------
>   4 files changed, 15 insertions(+), 12 deletions(-)
As I asked elsewhere, can we arrange for main.c to call run_command() 
prior to .get_ready, then sh and eval can still call setenv() during 
.get_ready (at a point where it will not be picked up by the --run 
process), more efficiently than copying the environ on every callback?

Or, even if we can't call setenv(), can we at least compute the 
alternative environment up front...
> +++ b/plugins/sh/call.c
> @@ -106,6 +106,7 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to
stdin */
>          const char **argv)                /* script + parameters */
>   {
>     const char *argv0 = argv[0]; /* script name, used in error messages */
> +  CLEANUP_FREE_STRING_LIST char **env = NULL;
>     pid_t pid = -1;
>     int status;
>     int ret = ERROR;
> @@ -122,6 +123,11 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to
stdin */
>   
>     debug_call (argv);
>   
> +  /* Copy the environment, and add $tmpdir. */
> +  env = copy_environ (environ, "tmpdir", tmpdir, NULL);
> +  if (env == NULL)
> +    goto error;
> +
...and pass the precomputed environ to call3() rather than re-computing 
it on every callback?

Can we add testsuite coverage for this?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org