Libguestfs - Aug 2020 - [nbdkit PATCH 5/5] sh, eval: Implement .default_export

More patches on the way for improving .list_exports signature and
adding .export_description, but this is the promised code showing
why nbdkit_string_intern is useful.  Patch 4 is somewhat RFC: we
could either add new API to take the boilerplate from:

foo_config(const char *key, const char *value) {
  if (strcmp (key, "file") == 0) {
    CLEANUP_FREE char *tmp = nbdkit_realpath (value);
    filename = nbdkit_string_intern (tmp);
    if (!filename)
      return -1;
  }
  ...

to the shorter:

foo_config(const char *key, const char *value) {
  if (strcmp (key, "file") == 0) {
    if ((filename = nbdkit_realpath_intern (value)) == NULL)
      return -1;
  }

by adding 'const char *nbdkit_realpath_intern(const char *name)',
or maybe we instead add a 'bool malloced' knob to nbdkit_string_intern,
passing true transfers ownership of an existing string, and passing
false forces nbdkit to copy (but exposes a bit more of a malloc/free
coupling between plugin and nbdkit):

foo_config(const char *key, const char *value) {
  if (strcmp (key, "file") == 0) {
    if ((filename = nbdkit_string_intern (nbdkit_realpath (value),
          true)) == NULL)
      return -1;
  }

Also, more than just the file plugin shares the pattern of copying
off a config name where we could simplify or eliminate .unload by
interning the string.

Eric Blake (5):
  api: Add .default_export
  server: Respond to NBD_INFO_NAME request
  api: Add nbdkit_string_intern helper
  file: Utilize nbdkit_string_intern
  sh, eval: Implement .default_export

 docs/nbdkit-filter.pod               |  37 ++++++---
 docs/nbdkit-plugin.pod               |  66 +++++++++++++++-
 plugins/eval/nbdkit-eval-plugin.pod  |   2 +
 plugins/sh/nbdkit-sh-plugin.pod      |  20 ++++-
 include/nbdkit-common.h              |   2 +
 include/nbdkit-filter.h              |   9 ++-
 include/nbdkit-plugin.h              |   1 +
 tests/Makefile.am                    |   4 +-
 server/internal.h                    |  14 +++-
 server/backend.c                     |  45 +++++++----
 server/connections.c                 |   2 +-
 server/exports.c                     |   8 +-
 server/filters.c                     |  12 +++
 server/main.c                        |  27 ++-----
 server/nbdkit.syms                   |   1 +
 server/plugins.c                     |  33 +++++---
 server/protocol-handshake-newstyle.c |  68 ++++++++++++++--
 server/public.c                      |  42 ++++++++++
 plugins/sh/methods.h                 |   1 +
 plugins/eval/eval.c                  |   2 +
 plugins/file/file.c                  |  20 ++---
 plugins/ondemand/ondemand.c          |  11 ++-
 plugins/sh/methods.c                 |  54 ++++++++++++-
 plugins/sh/sh.c                      |   1 +
 plugins/sh/example.sh                |   2 +-
 filters/log/log.c                    |  10 +--
 filters/tls-fallback/tls-fallback.c  |  12 ++-
 tests/test-eval-exports.sh           |  34 +++++---
 tests/test-export-info.sh            | 114 +++++++++++++++++++++++++++
 tests/test-tls-fallback.sh           |  11 ++-
 tests/test-layers-filter.c           |   9 +++
 tests/test-layers-plugin.c           |   8 ++
 tests/test-layers.c                  |  24 +++---
 33 files changed, 573 insertions(+), 133 deletions(-)
 create mode 100755 tests/test-export-info.sh

-- 
2.28.0

I'm about to add an 'exportname' filter, and in the process, I
noticed
a few shortcomings in our API.  Time to fix those before the 1.22
release locks our API in stone.  Overloading .list_exports in order to
determine a canonical export name at .open time is awkward; the two
uses (answering NBD_OPT_LIST for a full list, vs. remapping a client's
"" into a canonical name during .open) are orthogonal enough to
warrant separate plugin callbacks.  This will also make it easier to
express the notion of no default export (connecting to "" is an error)
at the same time as listing other exports.  Another consideration is
that when tls=1, the choice of export to expose pre-TLS vs. post-TLS
may differ, but without a call to .open yet, our just-added
nbdkit_is_tls() does not fit our preferred lifecycle, so this has to
be a parameter to the new .default_export.  We will alter the
signature of .list_exports soon; in the meantime, the bool
default_only parameter is now ignored.

Adding .default_export support to sh/eval is big enough for a separate
patch.

The ondemand plugin continues to advertise "" (and not
"default") in
its list of exports, but now reports a canonical name of "default"
when connecting to it.

The tls-fallback filter can now avoid leaking a default export name
through an insecure connection.

The test-layers.c changes demonstrate how fragile it is to maintain
our own naive client for probing which aspects of the plugin have been
reached; the test could probably be improved by using libnbd 1.4 APIs.
---
 docs/nbdkit-filter.pod               | 28 ++++++++++++-----
 docs/nbdkit-plugin.pod               | 37 +++++++++++++++++++++++
 include/nbdkit-filter.h              |  9 ++++--
 include/nbdkit-plugin.h              |  1 +
 server/internal.h                    |  7 +++--
 server/backend.c                     | 45 ++++++++++++++++++----------
 server/exports.c                     |  8 +----
 server/filters.c                     | 12 ++++++++
 server/plugins.c                     | 22 ++++++++++++--
 server/protocol-handshake-newstyle.c |  9 +++++-
 plugins/ondemand/ondemand.c          | 11 +++++--
 filters/tls-fallback/tls-fallback.c  | 12 +++++++-
 tests/test-eval-exports.sh           |  3 +-
 tests/test-layers-filter.c           |  9 ++++++
 tests/test-layers-plugin.c           |  8 +++++
 tests/test-layers.c                  | 24 ++++++++-------
 16 files changed, 192 insertions(+), 53 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index 94cebccd..dd667c0c 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -137,9 +137,9 @@ C<nxdata> passed to C<.open> has a stable
lifetime that lasts to the
 corresponding C<.close>, with all intermediate functions (such as
 C<.pread>) receiving the same value for convenience; the only
 exceptions where C<nxdata> is not reused are C<.config>,
-C<.config_complete>, C<.get_ready>, C<.after_fork>,
C<.preconnect> and
-C<.list_exports>, which are called outside the lifetime of a
-connection.
+C<.config_complete>, C<.get_ready>, C<.after_fork>,
C<.preconnect>,
+C<.list_exports>, and C<.default_export>, which are called outside
+the lifetime of a connection.

 =head2 Next config, open and close

@@ -355,7 +355,7 @@ from the layer below.  Without error checking it would look
like this:
    struct nbdkit_export e;
    char *name, *desc;

-   exports2 = nbdkit_exports_new (default_only);
+   exports2 = nbdkit_exports_new ();
    next_list_exports (nxdata, readonly, default_only, exports);
    for (i = 0; i < nbdkit_exports_count (exports2); ++i) {
      e = nbdkit_get_export (exports2, i);
@@ -376,11 +376,9 @@ an error message and return C<-1>.
 Two functions are provided to filters only for allocating and freeing
 the list:

- struct nbdkit_exports *nbdkit_exports_new (int default_only);
+ struct nbdkit_exports *nbdkit_exports_new (void);

-Allocates and returns a new, empty exports list.  The C<default_only>
-parameter should match whether the list is intended to grab the
-canonical name of the default export, or all exports.
+Allocates and returns a new, empty exports list.

 On error this function can return C<NULL>.  In this case it calls
 C<nbdkit_error> as required.  C<errno> will be set to a suitable
@@ -408,6 +406,20 @@ Returns the number of exports in the list.

 Returns a copy of the C<i>'th export.

+=head2 C<.default_export>
+
+ const char *default_export (nbdkit_next_default_export *next, void *nxdata,
+                             int readonly, int is_tls)
+
+This intercepts the plugin C<.default_export> method and can be used to
+alter the canonical export name used in place of the default
C<"">.
+
+The C<readonly> parameter matches what is passed to <.preconnect>
and
+C<.open>, and may be changed by the filter when calling into the
+plugin.  The C<is_tls> parameter informs the filter whether TLS
+negotiation has been completed by the client, but is not passed on to
+C<next> because it cannot be altered.
+
 =head2 C<.open>

  void * (*open) (nbdkit_next_open *next, void *nxdata,
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 5c641e83..64e3197b 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -245,6 +245,12 @@ Early in option negotiation the client may try to list the
exports
 served by the plugin, and plugins can optionally implement this
 callback to answer the client.  See L</EXPORT NAME> below.

+=item C<.default_export>
+
+During option negotiation, if the client requests the default export
+name (C<"">), this optional callback provides a canonical name
to
+use in its place prior to calling C<.open>.
+
 =item C<.open>

 A new client has connected and finished the NBD handshake.  TLS
@@ -709,6 +715,37 @@ Returning C<0> will send the list of exports back to
the client.  If
 there is an error, C<.list_exports> should call C<nbdkit_error>
with
 an error message and return C<-1>.

+=head2 C<.default_export>
+
+ const char *default_export (int readonly, int is_tls);
+
+This optional callback is called if the client tries to connect to
+the default export C<"">.  If the plugin does not supply this
callback,
+the connection continues with the empty name; if the plugin returns
+a string, nbdkit returns that name to clients that support it (see
+the C<NBD_INFO_NAME> response to C<NBD_OPT_GO>), and behaves as if
+the client had passed that string instead of an empty name.  Similarly,
+if the plugin does not supply a C<.list_exports> callback, the result
+of this callback determines what export name to advertise to a client
+requesting an export list.
+
+The C<readonly> flag informs the plugin that the server was started
+with the I<-r> flag on the command line, which is the same value
+passed to C<.preconnect> and C<.open>.  However, the NBD protocol
does
+not yet have a way to let the client advertise an intent to be
+read-only even when the server allows writes, so this parameter may
+not be as useful as it appears.
+
+The C<is_tls> flag informs the plugin whether the canonical name for
+the default export is being requested after the client has completed
+TLS negotiation.  When running the server in a mode that permits but
+not requires TLS, be careful that a default export name does not leak
+unintended information.
+
+If the plugin returns C<NULL>, the client is not permitted to connect
+to the default export.  However, this is not an error in the protocol,
+so it is not necessary to call C<nbdkit_error>.
+
 =head2 C<.open>

  void *open (int readonly);
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index b4024ae5..2c5b36be 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -66,8 +66,10 @@ typedef int nbdkit_next_get_ready (nbdkit_backend *nxdata);
 typedef int nbdkit_next_after_fork (nbdkit_backend *nxdata);
 typedef int nbdkit_next_preconnect (nbdkit_backend *nxdata, int readonly);
 typedef int nbdkit_next_list_exports (nbdkit_backend *nxdata, int readonly,
-                                      int default_only,
+                                      int ignored,
                                       struct nbdkit_exports *exports);
+typedef const char *nbdkit_next_default_export (nbdkit_backend *nxdata,
+                                                int readonly);
 typedef int nbdkit_next_open (nbdkit_backend *nxdata,
                               int readonly, const char *exportname);

@@ -136,7 +138,7 @@ struct nbdkit_export {
 };

 NBDKIT_EXTERN_DECL (struct nbdkit_exports *, nbdkit_exports_new,
-                    (int default_only));
+                    (void));
 NBDKIT_EXTERN_DECL (void, nbdkit_exports_free, (struct nbdkit_exports *));
 NBDKIT_EXTERN_DECL (size_t, nbdkit_exports_count,
                     (const struct nbdkit_exports *));
@@ -179,6 +181,9 @@ struct nbdkit_filter {
   int (*list_exports) (nbdkit_next_list_exports *next, nbdkit_backend *nxdata,
                        int readonly, int default_only,
                        struct nbdkit_exports *exports);
+  const char * (*default_export) (nbdkit_next_default_export *next,
+                                  nbdkit_backend *nxdata,
+                                  int readonly, int is_tls);

   void * (*open) (nbdkit_next_open *next, nbdkit_backend *nxdata,
                   int readonly, const char *exportname, int is_tls);
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index a5d85411..28e83757 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -142,6 +142,7 @@ struct nbdkit_plugin {

   int (*list_exports) (int readonly, int default_only,
                        struct nbdkit_exports *exports);
+  const char * (*default_export) (int readonly, int is_tls);
 };

 NBDKIT_EXTERN_DECL (void, nbdkit_set_error, (int err));
diff --git a/server/internal.h b/server/internal.h
index d043225a..8c8448e6 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -365,8 +365,9 @@ struct backend {
   void (*get_ready) (struct backend *);
   void (*after_fork) (struct backend *);
   int (*preconnect) (struct backend *, int readonly);
-  int (*list_exports) (struct backend *, int readonly, int default_only,
+  int (*list_exports) (struct backend *, int readonly, int ignored,
                        struct nbdkit_exports *exports);
+  const char *(*default_export) (struct backend *, int readonly, int is_tls);
   void *(*open) (struct backend *, int readonly, const char *exportname,
                  int is_tls);
   int (*prepare) (struct backend *, void *handle, int readonly);
@@ -413,9 +414,11 @@ extern void backend_unload (struct backend *b, void
(*unload) (void))
   __attribute__((__nonnull__ (1)));

 extern int backend_list_exports (struct backend *b, int readonly,
-                                 int default_only,
+                                 int ignored,
                                  struct nbdkit_exports *exports)
   __attribute__((__nonnull__ (1, 4)));
+extern const char *backend_default_export (struct backend *b, int readonly)
+  __attribute__((__nonnull__ (1)));
 /* exportname is only valid for this call and almost certainly will be
  * freed on return of this function, so backends must save the
  * exportname if they need to refer to it later.
diff --git a/server/backend.c b/server/backend.c
index 2ca40d61..c5eef456 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -166,13 +166,12 @@ backend_list_exports (struct backend *b, int readonly, int
default_only,
   struct handle *h = get_handle (conn, b->i);
   int r;

+  assert (!default_only); /* XXX Switch to is_tls... */
   controlpath_debug ("%s: list_exports readonly=%d default_only=%d",
                      b->name, readonly, default_only);

   assert (h->handle == NULL);
   assert ((h->state & HANDLE_OPEN) == 0);
-  if (default_only && h->default_exportname)
-    return nbdkit_add_export (exports, h->default_exportname, NULL);

   r = b->list_exports (b, readonly, default_only, exports);
   if (r == -1)
@@ -180,13 +179,34 @@ backend_list_exports (struct backend *b, int readonly, int
default_only,
   else {
     size_t count = nbdkit_exports_count (exports);
     controlpath_debug ("%s: list_exports returned %zu names",
b->name, count);
-    /* Best effort caching of default export name */
-    if (!h->default_exportname && count)
-      h->default_exportname = strdup (nbdkit_get_export (exports, 0).name);
   }
   return r;
 }

+const char *
+backend_default_export (struct backend *b, int readonly)
+{
+  GET_CONN;
+  struct handle *h = get_handle (conn, b->i);
+  const char *s;
+
+  controlpath_debug ("%s: default_export readonly=%d tls=%d",
+                     b->name, readonly, conn->using_tls);
+
+  if (h->default_exportname == NULL) {
+    assert (h->handle == NULL);
+    assert ((h->state & HANDLE_OPEN) == 0);
+    s = b->default_export (b, readonly, conn->using_tls);
+    if (s) {
+      /* Best effort caching */
+      h->default_exportname = strdup (s);
+      if (h->default_exportname == NULL)
+        return s;
+    }
+  }
+  return h->default_exportname;
+}
+
 int
 backend_open (struct backend *b, int readonly, const char *exportname)
 {
@@ -202,18 +222,13 @@ backend_open (struct backend *b, int readonly, const char
*exportname)
   if (readonly)
     h->can_write = 0;

-  /* Best-effort determination of the canonical name for default export */
+  /* Determine the canonical name for default export */
   if (!*exportname) {
-    if (!h->default_exportname) {
-      CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps = NULL;
-
-      exps = nbdkit_exports_new (true);
-      if (exps && b->list_exports (b, readonly, true, exps) == 0
&&
-          nbdkit_exports_count (exps))
-        h->default_exportname = strdup (nbdkit_get_export (exps, 0).name);
+    exportname = backend_default_export (b, readonly);
+    if (exportname == NULL) {
+      nbdkit_error ("default export (\"\") not permitted");
+      return -1;
     }
-    if (h->default_exportname)
-      exportname = h->default_exportname;
   }

   /* Most filters will call next_open first, resulting in
diff --git a/server/exports.c b/server/exports.c
index 3f819622..8d3faec4 100644
--- a/server/exports.c
+++ b/server/exports.c
@@ -52,12 +52,10 @@ DEFINE_VECTOR_TYPE(exports, struct nbdkit_export);

 struct nbdkit_exports {
   exports exports;
-
-  bool default_only;
 };

 struct nbdkit_exports *
-nbdkit_exports_new (int default_only)
+nbdkit_exports_new (void)
 {
   struct nbdkit_exports *r;

@@ -67,7 +65,6 @@ nbdkit_exports_new (int default_only)
     return NULL;
   }
   r->exports = (exports) empty_vector;
-  r->default_only = default_only != 0;
   return r;
 }

@@ -107,9 +104,6 @@ nbdkit_add_export (struct nbdkit_exports *exps,
 {
   struct nbdkit_export e = { NULL, NULL };

-  if (exps->default_only && exps->exports.size == 1)
-    return 0;
-
   if (exps->exports.size == MAX_EXPORTS) {
     nbdkit_error ("nbdkit_add_export: too many exports");
     errno = EINVAL;
diff --git a/server/filters.c b/server/filters.c
index 0cfae344..bb22be76 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -249,6 +249,17 @@ filter_list_exports (struct backend *b, int readonly, int
default_only,
   return backend_list_exports (b->next, readonly, default_only, exports);
 }

+static const char *
+filter_default_export (struct backend *b, int readonly, int is_tls)
+{
+  struct backend_filter *f = container_of (b, struct backend_filter, backend);
+
+  if (f->filter.default_export)
+    return f->filter.default_export (backend_default_export, b->next,
+                                     readonly, is_tls);
+  return backend_default_export (b->next, readonly);
+}
+
 static void *
 filter_open (struct backend *b, int readonly, const char *exportname,
              int is_tls)
@@ -555,6 +566,7 @@ static struct backend filter_functions = {
   .after_fork = filter_after_fork,
   .preconnect = filter_preconnect,
   .list_exports = filter_list_exports,
+  .default_export = filter_default_export,
   .open = filter_open,
   .prepare = filter_prepare,
   .finalize = filter_finalize,
diff --git a/server/plugins.c b/server/plugins.c
index 218764da..924533cb 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -162,6 +162,7 @@ plugin_dump_fields (struct backend *b)
   HAS (after_fork);
   HAS (preconnect);
   HAS (list_exports);
+  HAS (default_export);
   HAS (open);
   HAS (close);
   HAS (get_size);
@@ -282,15 +283,29 @@ static int
 plugin_list_exports (struct backend *b, int readonly, int default_only,
                      struct nbdkit_exports *exports)
 {
-  GET_CONN;
   struct backend_plugin *p = container_of (b, struct backend_plugin, backend);

-  if (!p->plugin.list_exports)
-    return nbdkit_add_export (exports, "", NULL);
+  if (!p->plugin.list_exports) {
+    const char *def = backend_default_export (b, readonly);
+    if (def == NULL)
+      return 0;
+    return nbdkit_add_export (exports, def, NULL);
+  }

   return p->plugin.list_exports (readonly, default_only, exports);
 }

+static const char *
+plugin_default_export (struct backend *b, int readonly, int is_tls)
+{
+  struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
+
+  if (!p->plugin.default_export)
+    return "";
+
+  return p->plugin.default_export (readonly, is_tls);
+}
+
 static void *
 plugin_open (struct backend *b, int readonly, const char *exportname,
              int is_tls)
@@ -757,6 +772,7 @@ static struct backend plugin_functions = {
   .after_fork = plugin_after_fork,
   .preconnect = plugin_preconnect,
   .list_exports = plugin_list_exports,
+  .default_export = plugin_default_export,
   .open = plugin_open,
   .prepare = plugin_prepare,
   .finalize = plugin_finalize,
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 8f41c7a8..6e5f3822 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -85,7 +85,7 @@ send_newstyle_option_reply_exportnames (uint32_t option)
   size_t i;
   CLEANUP_EXPORTS_FREE struct nbdkit_exports *exps = NULL;

-  exps = nbdkit_exports_new (false);
+  exps = nbdkit_exports_new ();
   if (exps == NULL)
     return send_newstyle_option_reply (option, NBD_REP_ERR_TOO_BIG);
   if (backend_list_exports (top, read_only, false, exps) == -1)
@@ -301,6 +301,7 @@ negotiate_handshake_newstyle_options (void)
   struct nbd_export_name_option_reply handshake_finish;
   const char *optname;
   uint64_t exportsize;
+  struct backend *b;

   for (nr_options = 0; nr_options < MAX_NR_OPTIONS; ++nr_options) {
     CLEANUP_FREE char *data = NULL;
@@ -445,6 +446,12 @@ negotiate_handshake_newstyle_options (void)
           return -1;
         conn->using_tls = true;
         debug ("using TLS on this connection");
+        /* Wipe out any cached default export name. */
+        for_each_backend (b) {
+          struct handle *h = get_handle (conn, b->i);
+          free (h->default_exportname);
+          h->default_exportname = NULL;
+        }
       }
       break;

diff --git a/plugins/ondemand/ondemand.c b/plugins/ondemand/ondemand.c
index f9d73ca6..5bd0ee70 100644
--- a/plugins/ondemand/ondemand.c
+++ b/plugins/ondemand/ondemand.c
@@ -219,6 +219,13 @@ ondemand_list_exports (int readonly, int default_only,
   return 0;
 }

+static const char *
+ondemand_default_export (int readonly, int is_tls)
+{
+  /* We always accept "" as an export name; canonicalize it to
"default". */
+  return "default";
+}
+
 struct handle {
   int fd;
   int64_t size;
@@ -362,8 +369,7 @@ ondemand_open (int readonly)
     nbdkit_error ("internal error: expected nbdkit_export_name () !=
NULL");
     goto error;
   }
-  if (strcmp (h->exportname, "") == 0)
-    h->exportname = "default";
+  assert (strcmp (h->exportname, "") != 0); /* see .default_export
*/

   /* Verify that the export name is valid. */
   if (strlen (h->exportname) > NAME_MAX ||
@@ -621,6 +627,7 @@ static struct nbdkit_plugin plugin = {
   .get_ready         = ondemand_get_ready,

   .list_exports      = ondemand_list_exports,
+  .default_export    = ondemand_default_export,

   .can_multi_conn    = ondemand_can_multi_conn,
   .can_trim          = ondemand_can_trim,
diff --git a/filters/tls-fallback/tls-fallback.c
b/filters/tls-fallback/tls-fallback.c
index 822748b4..0fcc2bcf 100644
--- a/filters/tls-fallback/tls-fallback.c
+++ b/filters/tls-fallback/tls-fallback.c
@@ -76,6 +76,15 @@ tls_fallback_get_ready (nbdkit_next_get_ready *next, void
*nxdata,

 /* TODO: list_exports needs is_tls parameter */

+static const char *
+tls_fallback_default_export (nbdkit_next_default_export *next, void *nxdata,
+                             int readonly, int is_tls)
+{
+  if (!is_tls)
+    return "";
+  return next (nxdata, readonly);
+}
+
 /* Helper for determining if this connection is insecure.  This works
  * because we can treat all handles on a binary basis: secure or
  * insecure, which lets .open get away without allocating a more
@@ -183,7 +192,8 @@ static struct nbdkit_filter filter = {
   .config            = tls_fallback_config,
   .config_help       = tls_fallback_config_help,
   .get_ready         = tls_fallback_get_ready,
-  /* XXX .init_exports needs is_tls parameter */
+  /* XXX .list_exports needs is_tls parameter */
+  .default_export    = tls_fallback_default_export,
   .open              = tls_fallback_open,
   .get_size          = tls_fallback_get_size,
   .can_write         = tls_fallback_can_write,
diff --git a/tests/test-eval-exports.sh b/tests/test-eval-exports.sh
index aa694aaa..7c946378 100755
--- a/tests/test-eval-exports.sh
+++ b/tests/test-eval-exports.sh
@@ -67,7 +67,8 @@ do_nbdkit ()
     # Check how the default export name is handled
     # nbdinfo currently makes multiple connections, so we can't use the
     # long-running server for validating default export name.
-    nbdkit -U - -v eval list_exports="cat
'$PWD/eval-exports.list'" \
+    # XXX FIXME: requires .default_export in eval
+    : || nbdkit -U - -v eval list_exports="cat
'$PWD/eval-exports.list'" \
       open='[ "$3" = "'"$1"'" ] || {
echo EINVAL wrong export >&2; exit 1; }' \
       get_size='echo 0' --run 'nbdsh -u "$uri" -c
"exit()"'
     # Check what exports are listed
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
index 3f295588..f6acb448 100644
--- a/tests/test-layers-filter.c
+++ b/tests/test-layers-filter.c
@@ -115,6 +115,14 @@ test_layers_filter_list_exports (nbdkit_next_list_exports
*next, void *nxdata,
   return next (nxdata, readonly, default_only, exports);
 }

+static const char *
+test_layers_filter_default_export (nbdkit_next_default_export *next,
+                                   void *nxdata, int readonly, int is_tls)
+{
+  DEBUG_FUNCTION;
+  return next (nxdata, readonly);
+}
+
 static void *
 test_layers_filter_open (nbdkit_next_open *next, void *nxdata,
                          int readonly, const char *exportname, int is_tls)
@@ -380,6 +388,7 @@ static struct nbdkit_filter filter = {
   .after_fork        = test_layers_filter_after_fork,
   .preconnect        = test_layers_filter_preconnect,
   .list_exports      = test_layers_filter_list_exports,
+  .default_export    = test_layers_filter_default_export,
   .open              = test_layers_filter_open,
   .close             = test_layers_filter_close,
   .prepare           = test_layers_filter_prepare,
diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c
index 1dfd069e..254244eb 100644
--- a/tests/test-layers-plugin.c
+++ b/tests/test-layers-plugin.c
@@ -101,6 +101,13 @@ test_layers_plugin_list_exports (int readonly, int
default_only,
   return nbdkit_add_export (exports, "", NULL);
 }

+static const char *
+test_layers_plugin_default_export (int readonly, int is_tls)
+{
+  DEBUG_FUNCTION;
+  return "";
+}
+
 static void *
 test_layers_plugin_open (int readonly)
 {
@@ -257,6 +264,7 @@ static struct nbdkit_plugin plugin = {
   .after_fork        = test_layers_plugin_after_fork,
   .preconnect        = test_layers_plugin_preconnect,
   .list_exports      = test_layers_plugin_list_exports,
+  .default_export    = test_layers_plugin_default_export,
   .open              = test_layers_plugin_open,
   .close             = test_layers_plugin_close,
   .get_size          = test_layers_plugin_get_size,
diff --git a/tests/test-layers.c b/tests/test-layers.c
index dd826f36..fa7730e6 100644
--- a/tests/test-layers.c
+++ b/tests/test-layers.c
@@ -326,19 +326,21 @@ main (int argc, char *argv[])
      "test_layers_plugin_preconnect",
      NULL);

-  /* list_exports methods called in outer-to-inner order, complete
-   * in inner-to-outer order.  But since we didn't send NBD_OPT_LIST,
-   * the outer filter does not expose a list; rather, the rest of the
-   * chain is used to resolve the canonical name of the default export.
+  /* XXX We should test NBD_OPT_INFO here for coverage of
+   * .list_exports.  However it would be easier to do by using libnbd
+   * than open-coding a naive client.
    */
+
+  /* .default_export methods called in outer-to-inner order. */
   log_verify_seen_in_order
-    ("filter3: test_layers_filter_list_exports",
-     "testlayersfilter2: list_exports",
-     "filter2: test_layers_filter_list_exports",
-     "testlayersfilter1: list_exports",
-     "filter1: test_layers_filter_list_exports",
-     "testlayersplugin: list_exports",
-     "test_layers_plugin_list_exports",
+    ("testlayersfilter3: default_export",
+     "filter3: test_layers_filter_default_export",
+     "testlayersfilter2: default_export",
+     "filter2: test_layers_filter_default_export",
+     "testlayersfilter1: default_export",
+     "filter1: test_layers_filter_default_export",
+     "testlayersplugin: default_export",
+     "test_layers_plugin_default_export",
      NULL);

   /* open methods called in outer-to-inner order, but thanks to next
-- 
2.28.0

The NBD spec says that additional information in response to
NBD_OPT_GO or NBD_OPT_INFO, such as NBD_INFO_NAME, is optional, but
also recommends that if a client hints about wanting a particular
piece of information, that it is better for the server to reply to
those known hints.  This is also the the only way for a client to
learn the canonical name that is used when connecting to the default
export name.

libnbd 1.4 is adding new APIs to request the client hint, and to track
what the server replies with, making this easy enough to test.
However, at present, the ondemand plugin is the only one to advertise
an alternate name for the default export, so the test will be further
enhanced when .default_export is plumbed through the sh plugin.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 tests/Makefile.am                    |   4 +-
 server/protocol-handshake-newstyle.c |  59 +++++++++++++-
 tests/test-export-info.sh            | 114 +++++++++++++++++++++++++++
 3 files changed, 171 insertions(+), 6 deletions(-)
 create mode 100755 tests/test-export-info.sh

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 2db56ded..29c4811b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -491,8 +491,8 @@ TESTS += test-eflags.sh
 EXTRA_DIST += test-eflags.sh

 # Test export name.
-TESTS += test-export-name.sh
-EXTRA_DIST += test-export-name.sh
+TESTS += test-export-name.sh test-export-info.sh
+EXTRA_DIST += test-export-name.sh test-export-info.sh

 # cdi plugin test.
 TESTS += test-cdi.sh
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 6e5f3822..1777219f 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -156,6 +156,36 @@ send_newstyle_option_reply_info_export (uint32_t option,
uint32_t reply,
   return 0;
 }

+/* Can be used for NBD_INFO_NAME and NBD_INFO_DESCRIPTION. */
+static int
+send_newstyle_option_reply_info_str (uint32_t option, uint32_t reply,
+                                     uint16_t info, const char *str,
+                                     size_t len)
+{
+  GET_CONN;
+  struct nbd_fixed_new_option_reply fixed_new_option_reply;
+  struct nbd_fixed_new_option_reply_info_name_or_desc name;
+
+  if (len == -1)
+    len = strlen (str);
+
+  fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC);
+  fixed_new_option_reply.option = htobe32 (option);
+  fixed_new_option_reply.reply = htobe32 (reply);
+  fixed_new_option_reply.replylen = htobe32 (sizeof info + len);
+  name.info = htobe16 (info);
+
+  if (conn->send (&fixed_new_option_reply,
+                  sizeof fixed_new_option_reply, SEND_MORE) == -1 ||
+      conn->send (&name, sizeof name, SEND_MORE) == -1 ||
+      conn->send (str, len, 0) == -1) {
+    nbdkit_error ("write: %s: %m", name_of_nbd_opt (option));
+    return -1;
+  }
+
+  return 0;
+}
+
 static int
 send_newstyle_option_reply_meta_context (uint32_t option, uint32_t reply,
                                          uint32_t context_id,
@@ -533,16 +563,37 @@ negotiate_handshake_newstyle_options (void)
                                                     exportsize) == -1)
           return -1;

-        /* For now we ignore all other info requests (but we must
-         * ignore NBD_INFO_EXPORT if it was requested, because we
-         * replied already above).  Therefore this loop doesn't do
-         * much at the moment.
+        /* For now we send NBD_INFO_NAME if requested, and ignore all
+         * other info requests (including NBD_INFO_EXPORT if it was
+         * requested, because we replied already above).
+         * XXX NBD_INFO_DESCRIPTION is easy once we add .export_description.
          */
         for (i = 0; i < nrinfos; ++i) {
           memcpy (&info, &data[4 + exportnamelen + 2 + i*2], 2);
           info = be16toh (info);
           switch (info) {
           case NBD_INFO_EXPORT: /* ignore - reply sent above */ break;
+          case NBD_INFO_NAME:
+            {
+              const char *name = &data[4];
+              size_t namelen = exportnamelen;
+
+              if (exportnamelen == 0) {
+                name = backend_default_export (top, read_only);
+                if (!name) {
+                  debug ("newstyle negotiation: %s: "
+                         "NBD_INFO_NAME: no name to send", optname);
+                  break;
+                }
+                namelen = -1;
+              }
+              if (send_newstyle_option_reply_info_str (option,
+                                                       NBD_REP_INFO,
+                                                       NBD_INFO_NAME,
+                                                       name, namelen) == -1)
+                return -1;
+            }
+            break;
           default:
             debug ("newstyle negotiation: %s: "
                    "ignoring NBD_INFO_* request %u (%s)",
diff --git a/tests/test-export-info.sh b/tests/test-export-info.sh
new file mode 100755
index 00000000..f9475e04
--- /dev/null
+++ b/tests/test-export-info.sh
@@ -0,0 +1,114 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019-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.
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin sh
+requires nbdsh -c 'print (h.set_full_info)'
+
+sock=`mktemp -u`
+files="$sock export-info.pid"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Create an nbdkit sh plugin for checking NBD_INFO replies to NBD_OPT_GO.
+# XXX Update when .default_export and .export_description are implemented in sh
+start_nbdkit -P export-info.pid -U $sock \
+             sh - <<'EOF'
+case "$1" in
+    default_export) echo hello ;;
+    open) echo "$3" ;;
+    export_description) echo "$2 world" ;;
+    get_size) echo 0 ;;
+    *) exit 2 ;;
+esac
+EOF
+
+# Without client request, nothing is advertised
+export sock
+nbdsh -c '
+import os
+
+h.set_opt_mode (True)
+h.connect_unix (os.environ["sock"])
+
+h.set_export_name ("a")
+h.opt_info ()
+try:
+  h.get_canonical_export_name ()
+  assert False
+except nbd.Error as ex:
+  pass
+
+h.set_export_name ("")
+h.opt_info ()
+try:
+  h.get_canonical_export_name ()
+  assert False
+except nbd.Error as ex:
+  pass
+
+h.opt_go ()
+try:
+  h.get_canonical_export_name ()
+  assert False
+except nbd.Error as ex:
+  pass
+
+h.shutdown ()
+'
+
+# With client request, reflect the export name back
+export sock
+nbdsh -c '
+import os
+
+h.set_opt_mode (True)
+h.set_full_info (True)
+h.connect_unix (os.environ["sock"])
+
+h.set_export_name ("a")
+h.opt_info ()
+assert h.get_canonical_export_name () == "a"
+
+h.set_export_name ("")
+h.opt_info ()
+# XXX Adjust once default export works
+assert h.get_canonical_export_name () == ""
+
+h.opt_go ()
+assert h.get_canonical_export_name () == ""
+
+h.shutdown ()
+'
-- 
2.28.0

Implementing .default_export with its 'const char *' return is tricky
in the sh plugin: we must return dynamic memory, but must avoid a
use-after-free.  And we don't want to change the return type of
.default_export to 'char *', because that would make our choice of
malloc()/free() part of the API, preventing either nbdkit or a plugin
from experimenting with an alternate allocator implementation.
Elsewhere, we have done things like nbdkit_extents_new(), so that even
though the client is directing allocation, the actual call to malloc()
is done by nbdkit proper, so that nbdkit later calling free() does not
tie the plugin's hands, and nbdkit could change its underlying
allocation without breaking ABI.  (Note that nbdkit_realpath() and
nbdkit_absolute_path() require the caller to use free(), but as they
are used during .config, it's less of a burden for plugins to take
care of that in .unload.)

We could document that the burden is on the plugin to avoid the memory
leak, by making sh track all strings it returns, then finally clean
them up during .unload.  But this is still a memory leak in the case
of .default_export, which can be called multiple times when there are
multiple clients, and presumably with different values per client
call; better would be freeing the memory at .close when each client
goes away.  Except that .default_export is called before .open, and
therefore before we have access to the handle struct that will
eventually make it to .close.

So, the solution is to let nbdkit track an alternate string on our
behalf, where nbdkit then cleans it up as the client goes away.

There are several places in the existing code base that can also take
advantage of this copying functionality, including in filters that
want to pass altered strings to .config while still obeying lifetime
rules.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-filter.pod  |  9 ++++++---
 docs/nbdkit-plugin.pod  | 31 +++++++++++++++++++++++++++---
 include/nbdkit-common.h |  2 ++
 server/internal.h       |  7 ++++++-
 server/connections.c    |  2 +-
 server/main.c           | 27 ++++++++------------------
 server/nbdkit.syms      |  1 +
 server/plugins.c        | 11 +++--------
 server/public.c         | 42 +++++++++++++++++++++++++++++++++++++++++
 filters/log/log.c       | 10 +++-------
 10 files changed, 100 insertions(+), 42 deletions(-)

diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index dd667c0c..286cdced 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -183,7 +183,8 @@ You can modify parameters when you call the C<next>
function.  However
 be careful when modifying strings because for some methods
 (eg. C<.config>) the plugin may save the string pointer that you pass
 along.  So you may have to ensure that the string is not freed for the
-lifetime of the server.
+lifetime of the server; you may find C<nbdkit_string_intern> helpful
+for avoiding a memory leak while still obeying lifecycle constraints.

 Note that if your filter registers a callback but in that callback it
 doesn't call the C<next> function then the corresponding method in
the
@@ -450,8 +451,10 @@ requires write access to the underlying device in case a
journal needs
 to be replayed for consistency as part of the mounting process.

 The C<exportname> string is only guaranteed to be available during the
-call.  If the filter needs to use it (other than immediately passing
-it down to the next layer) it must take a copy.  The C<exportname> and
+call (different than the lifetime for the return of C<nbdkit_export_name>
+used by plugins).  If the filter needs to use it (other than immediately
+passing it down to the next layer) it must take a copy, although
+C<nbdkit_string_intern> is useful for this task.  The C<exportname>
and
 C<is_tls> parameters are provided so that filters do not need to use
 the plugin-only interfaces of C<nbdkit_export_name> and
 C<nbdkit_is_tls>.
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 64e3197b..eaa5edba 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -744,7 +744,10 @@ unintended information.

 If the plugin returns C<NULL>, the client is not permitted to connect
 to the default export.  However, this is not an error in the protocol,
-so it is not necessary to call C<nbdkit_error>.
+so it is not necessary to call C<nbdkit_error>.  If the callback will
+not be returning a compile-time constant string, you may find
+C<nbdkit_string_intern> helpful for returning a value that avoids a
+memory leak.

 =head2 C<.open>

@@ -1500,8 +1503,9 @@ content.

 Return the optional NBD export name if one was negotiated with the
 current client (this uses thread-local magic so no parameter is
-required).  The returned string is only valid while the client is
-connected, so if you need to store it in the plugin you must copy it.
+required).  The returned string is valid at least through the
+C<.close> of the current connection, but if you need to store it
+in the plugin for use by more than one client you must copy it.

 The export name is a free-form text string, it is not necessarily a
 path or filename and it does not need to begin with a C<'/'>
@@ -1512,6 +1516,27 @@ client data, be cautious when parsing it.>

 On error, C<nbdkit_error> is called and the call returns C<NULL>.

+=head1 STRING LIFETIME
+
+Some callbacks are specified to return C<const char *>, even when a
+plugin may not have a suitable compile-time constant to return.
+Returning dynamically-allocated memory for such a callback would
+induce a memory leak or otherwise complicate the plugin to perform
+additional bookkeeping.  For these cases, nbdkit provides a
+convenience function for creating a copy of a string for better
+lifetime management.
+
+ const char *nbdkit_string_intern (const char *str);
+
+Returns a copy of str, so that the caller may reclaim str and use the
+copy in its place.  If the copy is created outside the scope of a
+connection (such as during C<.load>), the lifetime of the copy will
+last at least through C<.unload>; if it was created within a
+connection (such as during C<.preconnect>, C<.default_export>, or
+C<.open>), the lifetime will last at least through C<.close>.
+
+On error, C<nbdkit_error> is called and the call returns C<NULL>.
+
 =head1 AUTHENTICATION

 A server may use C<nbdkit_is_tls> to limit which export names work
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index c377e18d..533820c4 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -120,6 +120,8 @@ NBDKIT_EXTERN_DECL (int, nbdkit_nanosleep, (unsigned sec,
unsigned nsec));
 NBDKIT_EXTERN_DECL (int, nbdkit_peer_name,
                     (struct sockaddr *addr, socklen_t *addrlen));
 NBDKIT_EXTERN_DECL (void, nbdkit_shutdown, (void));
+NBDKIT_EXTERN_DECL (const char *, nbdkit_string_intern,
+                    (const char *str));

 struct nbdkit_extents;
 NBDKIT_EXTERN_DECL (int, nbdkit_add_extent,
diff --git a/server/internal.h b/server/internal.h
index 8c8448e6..9993d92a 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -237,6 +237,7 @@ reset_handle (struct handle *h)
   h->can_cache = -1;
 }

+DEFINE_VECTOR_TYPE(string_vector, char *);
 struct connection {
   pthread_mutex_t request_lock;
   pthread_mutex_t read_lock;
@@ -258,8 +259,9 @@ struct connection {
   bool structured_replies;
   bool meta_context_base_allocation;

+  string_vector interns;
   char *exportname_from_set_meta_context;
-  char *exportname;
+  const char *exportname;

   int sockin, sockout;
   connection_recv_function recv;
@@ -298,6 +300,9 @@ extern int protocol_recv_request_send_reply (void);
  */
 #define base_allocation_id 1

+/* public.c */
+extern void free_interns (void);
+
 /* crypto.c */
 #define root_tls_certificates_dir sysconfdir "/pki/" PACKAGE_NAME
 extern void crypto_init (bool tls_set_on_cli);
diff --git a/server/connections.c b/server/connections.c
index 67a68469..d9f685c9 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -360,7 +360,7 @@ free_connection (struct connection *conn)
   pthread_mutex_destroy (&conn->status_lock);

   free (conn->exportname_from_set_meta_context);
-  free (conn->exportname);
+  free_interns ();

   /* This is needed in order to free a field in struct handle. */
   for_each_backend (b)
diff --git a/server/main.c b/server/main.c
index 17c4c324..9cb12b59 100644
--- a/server/main.c
+++ b/server/main.c
@@ -631,15 +631,9 @@ main (int argc, char *argv[])
    *
    * Keys must live for the life of nbdkit.  Since we want to avoid
    * modifying argv (so that /proc/PID/cmdline remains sane) but we
-   * need to create a key from argv[i] = "key=value" we must save the
-   * keys in an array which is freed at the end of main().
+   * need to create a key from argv[i] = "key=value" we must intern
+   * the keys, which are then freed at the end of main().
    */
-  char **keys = calloc (argc, sizeof (char *));
-  if (keys == NULL) {
-    perror ("calloc");
-    exit (EXIT_FAILURE);
-  }
-
   magic_config_key = top->magic_config_key (top);
   for (i = 0; optind < argc; ++i, ++optind) {
     size_t n;
@@ -647,12 +641,11 @@ main (int argc, char *argv[])
     p = strchr (argv[optind], '=');
     n = p - argv[optind];
     if (p && is_config_key (argv[optind], n)) { /* Is it key=value? */
-      keys[optind] = strndup (argv[optind], n);
-      if (keys[optind] == NULL) {
-        perror ("strndup");
+      CLEANUP_FREE char *key = strndup (argv[optind], n);
+      const char *safekey = nbdkit_string_intern (key);
+      if (safekey == NULL)
         exit (EXIT_FAILURE);
-      }
-      top->config (top, keys[optind], p+1);
+      top->config (top, safekey, p+1);
     }
     else if (magic_config_key == NULL) {
       if (i == 0)               /* magic script parameter */
@@ -677,9 +670,7 @@ main (int argc, char *argv[])
   if (dump_plugin) {
     top->dump_fields (top);
     top->free (top);
-    for (i = 1; i < argc; ++i)
-      free (keys[i]);
-    free (keys);
+    free_interns ();
     exit (EXIT_SUCCESS);
   }

@@ -717,9 +708,7 @@ main (int argc, char *argv[])
   crypto_free ();
   close_quit_pipe ();

-  for (i = 1; i < argc; ++i)
-    free (keys[i]);
-  free (keys);
+  free_interns ();

   /* Note: Don't exit here, otherwise this won't work when compiled
    * for libFuzzer.
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index a67669b7..9e293444 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -73,6 +73,7 @@
     nbdkit_set_error;
     nbdkit_shutdown;
     nbdkit_stdio_safe;
+    nbdkit_string_intern;
     nbdkit_vdebug;
     nbdkit_verror;

diff --git a/server/plugins.c b/server/plugins.c
index 924533cb..3bc50bc7 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -329,17 +329,13 @@ plugin_open (struct backend *b, int readonly, const char
*exportname,
    * will still need to save the export name in the handle because of
    * the lifetime issue.
    */
-  conn->exportname = strdup (exportname);
-  if (conn->exportname == NULL) {
-    nbdkit_error ("strdup: %m");
+  conn->exportname = nbdkit_string_intern (exportname);
+  if (conn->exportname == NULL)
     return NULL;
-  }

   r = p->plugin.open (readonly);
-  if (r == NULL) {
-    free (conn->exportname);
+  if (r == NULL)
     conn->exportname = NULL;
-  }
   return r;
 }

@@ -368,7 +364,6 @@ plugin_close (struct backend *b, void *handle)

   if (handle && p->plugin.close)
     p->plugin.close (handle);
-  free (conn->exportname);
   conn->exportname = NULL;
 }

diff --git a/server/public.c b/server/public.c
index d10d466e..39a7a3d8 100644
--- a/server/public.c
+++ b/server/public.c
@@ -726,3 +726,45 @@ nbdkit_peer_name (struct sockaddr *addr, socklen_t
*addrlen)

   return 0;
 }
+
+/* Functions for manipulating intern'd strings. */
+
+static string_vector global_interns;
+
+void
+free_interns (void)
+{
+  struct connection *conn = threadlocal_get_conn ();
+  string_vector *list = conn ? &conn->interns : &global_interns;
+
+  string_vector_iter (list, (void *) free);
+  string_vector_reset (list);
+}
+
+const char *
+nbdkit_string_intern (const char *str)
+{
+  struct connection *conn = threadlocal_get_conn ();
+  string_vector *list = conn ? &conn->interns : &global_interns;
+  char *copy;
+
+  if (str == NULL) {
+    nbdkit_error ("nbdkit_string_intern: no string given");
+    errno = EINVAL;
+    return NULL;
+  }
+
+  copy = strdup (str);
+  if (copy == NULL) {
+    nbdkit_error ("strdup: %m");
+    return NULL;
+  }
+
+  if (string_vector_append (list, copy) == -1) {
+    nbdkit_error ("strdup: %m");
+    free (copy);
+    return NULL;
+  }
+
+  return copy;
+}
diff --git a/filters/log/log.c b/filters/log/log.c
index 71c21211..483c4a15 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -134,7 +134,7 @@ log_get_ready (nbdkit_next_get_ready *next, void *nxdata,
int thread_model)
 struct handle {
   uint64_t connection;
   uint64_t id;
-  char *exportname;
+  const char *exportname;
   int tls;
 };

@@ -305,9 +305,8 @@ log_open (nbdkit_next_open *next, void *nxdata,
    * it in log_prepare.  We must take a copy because this string has a
    * short lifetime.
    */
-  h->exportname = strdup (exportname);
+  h->exportname = nbdkit_string_intern (exportname);
   if (h->exportname == NULL) {
-    nbdkit_error ("strdup: %m");
     free (h);
     return NULL;
   }
@@ -322,10 +321,7 @@ log_open (nbdkit_next_open *next, void *nxdata,
 static void
 log_close (void *handle)
 {
-  struct handle *h = handle;
-
-  free (h->exportname);
-  free (h);
+  free (handle);
 }

 static int
-- 
2.28.0

Instead of needing to provide .unload, we can let nbdkit manage the
lifetime for us.  However, if we like this approach, it may be wiser
to introduce new variants of nbdkit_realpath/absolute_path which
return const char * already intern'd.

Signed-off-by: Eric Blake <eblake@redhat.com>
---

See the cover letter: if we like this, there are several other plugins
that could also reduce their .unload, or maybe we want to add
convenience functions to make the combo
'nbdkit_realpath/nbdkit_string_intern' simpler.

 plugins/file/file.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/plugins/file/file.c b/plugins/file/file.c
index 08418194..6a0aad93 100644
--- a/plugins/file/file.c
+++ b/plugins/file/file.c
@@ -64,8 +64,8 @@
 #define fdatasync fsync
 #endif

-static char *filename = NULL;
-static char *directory = NULL;
+static const char *filename = NULL;
+static const char *directory = NULL;

 /* posix_fadvise mode: -1 = don't set it, or POSIX_FADV_*. */
 static int fadvise_mode @@ -91,13 +91,6 @@ is_enotsup (int err)
   return err == ENOTSUP || err == EOPNOTSUPP;
 }

-static void
-file_unload (void)
-{
-  free (filename);
-  free (directory);
-}
-
 /* Called for each key=value passed on the command line.  This plugin
  * only accepts file=<filename> and dir=<dirname>, where exactly
  * one is required.
@@ -111,15 +104,15 @@ file_config (const char *key, const char *value)
    * existence checks to the last possible moment.
    */
   if (strcmp (key, "file") == 0) {
-    free (filename);
-    filename = nbdkit_realpath (value);
+    CLEANUP_FREE char *tmp = nbdkit_realpath (value);
+    filename = nbdkit_string_intern (tmp);
     if (!filename)
       return -1;
   }
   else if (strcmp (key, "directory") == 0 ||
            strcmp (key, "dir") == 0) {
-    free (directory);
-    directory = nbdkit_realpath (value);
+    CLEANUP_FREE char *tmp = nbdkit_realpath (value);
+    directory = nbdkit_string_intern (value);
     if (!directory)
       return -1;
   }
@@ -812,7 +805,6 @@ static struct nbdkit_plugin plugin = {
   .name              = "file",
   .longname          = "nbdkit file plugin",
   .version           = PACKAGE_VERSION,
-  .unload            = file_unload,
   .config            = file_config,
   .config_complete   = file_config_complete,
   .config_help       = file_config_help,
-- 
2.28.0

Use the recently added nbdkit_string_intern to make this possible.
Testsuite coverage is added in both tls-fallback as well as a cleanup
to test-eval-exports.sh.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/eval/nbdkit-eval-plugin.pod |  2 ++
 plugins/sh/nbdkit-sh-plugin.pod     | 20 +++++++++--
 plugins/sh/methods.h                |  1 +
 plugins/eval/eval.c                 |  2 ++
 plugins/sh/methods.c                | 54 +++++++++++++++++++++++++++--
 plugins/sh/sh.c                     |  1 +
 plugins/sh/example.sh               |  2 +-
 tests/test-eval-exports.sh          | 35 ++++++++++++-------
 tests/test-tls-fallback.sh          | 11 +++---
 9 files changed, 107 insertions(+), 21 deletions(-)

diff --git a/plugins/eval/nbdkit-eval-plugin.pod
b/plugins/eval/nbdkit-eval-plugin.pod
index 7126c6de..eed97f11 100644
--- a/plugins/eval/nbdkit-eval-plugin.pod
+++ b/plugins/eval/nbdkit-eval-plugin.pod
@@ -110,6 +110,8 @@ features):

 =item B<list_exports=>SCRIPT

+=item B<default_export=>SCRIPT
+
 =item B<open=>SCRIPT

 =item B<pread=>SCRIPT
diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index 07d90b57..b827b658 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -315,8 +315,24 @@ export name or description, although this could be possible
under a
 new mode supporting escape sequences.

 This method is I<not> required; if it is absent, the list of exports
-advertised by nbdkit will be the single export with the empty string
-as a name and no description.
+advertised by nbdkit will be the single name result of C<default_export>
+and no description.
+
+=item C<default_export>
+
+ /path/to/script default_export <readonly> <tls>
+
+The C<readonly> and C<tls> parameters will be C<true> or
C<false>.
+
+On success this should print a name on stdout to use in place of the
+default export C<"">, then exit with code C<0>.  For
convenience, the
+output can be any of the list forms recognized by C<list_exports>, in
+which case the first listed export name is used, and where an empty
+list uses C<"">.  Given the current set of recognized export
lists, it
+is not possible for the resulting name to include a newline.
+
+This method is I<not> required; if it is absent, the default export
+name will be the empty string, C<"">.

 =item C<open>

diff --git a/plugins/sh/methods.h b/plugins/sh/methods.h
index 69017fa4..3fd4ef42 100644
--- a/plugins/sh/methods.h
+++ b/plugins/sh/methods.h
@@ -46,6 +46,7 @@ extern int sh_after_fork (void);
 extern int sh_preconnect (int readonly);
 extern int sh_list_exports (int readonly, int default_only,
                             struct nbdkit_exports *exports);
+extern const char *sh_default_export (int readonly, int is_tls);
 extern void *sh_open (int readonly);
 extern void sh_close (void *handle);
 extern int64_t sh_get_size (void *handle);
diff --git a/plugins/eval/eval.c b/plugins/eval/eval.c
index 2bd5e79f..a123734e 100644
--- a/plugins/eval/eval.c
+++ b/plugins/eval/eval.c
@@ -68,6 +68,7 @@ static const char *known_methods[] = {
   "close",
   "config",
   "config_complete",
+  "default_export",
   "dump_plugin",
   "extents",
   "flush",
@@ -395,6 +396,7 @@ static struct nbdkit_plugin plugin = {

   .preconnect        = sh_preconnect,
   .list_exports      = sh_list_exports,
+  .default_export    = sh_default_export,
   .open              = sh_open,
   .close             = sh_close,

diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index 07fb3e48..a2631b39 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -241,7 +241,9 @@ parse_exports (const char *script,
 {
   const char *n, *d, *p, *q;

-  /* The first line determines how to parse the rest of s */
+  /* The first line determines how to parse the rest of s.  Keep
+   * sh_default_export in sync with this.
+   */
   if ((p = skip_prefix (s, "INTERLEAVED\n")) != NULL) {
     n = p;
     while ((d = strchr (n, '\n')) != NULL) {
@@ -310,13 +312,18 @@ sh_list_exports (int readonly, int default_only,
                          default_only ? "true" : "false",
NULL };
   CLEANUP_FREE char *s = NULL;
   size_t slen;
+  const char *def;

   switch (call_read (&s, &slen, args)) {
   case OK:
     return parse_exports (script, s, slen, exports);

   case MISSING:
-    return nbdkit_add_export (exports, "", NULL);
+    /* Match what is done for a C plugin. */
+    def = sh_default_export (readonly, nbdkit_is_tls ());
+    if (!def)
+      return 0;
+    return nbdkit_add_export (exports, def, NULL);

   case ERROR:
     return -1;
@@ -331,6 +338,49 @@ sh_list_exports (int readonly, int default_only,
   }
 }

+const char *
+sh_default_export (int readonly, int is_tls)
+{
+  const char *method = "default_export";
+  const char *script = get_script (method);
+  const char *args[] = { script, method, readonly ? "true" :
"false",
+                         is_tls ? "true" : "false", NULL };
+  CLEANUP_FREE char *s = NULL;
+  size_t slen;
+  const char *p, *n;
+  CLEANUP_FREE char *name = NULL;
+
+  switch (call_read (&s, &slen, args)) {
+  case OK:
+    /* The first line determines how to parse the rest of s.  For now,
+     * all export modes treat the next line as the first export.
+     */
+    if ((p = skip_prefix (s, "INTERLEAVED\n")) != NULL ||
+        (p = skip_prefix (s, "NAMES+DESCRIPTIONS\n")) != NULL ||
+        (p = skip_prefix (s, "NAMES\n")) != NULL)
+      ;
+    else
+      p = s;
+    n = strchr (p, '\n') ?: s + slen;
+    name = strndup (p, n - p);
+    return nbdkit_string_intern (name);
+
+  case MISSING:
+    return nbdkit_string_intern ("");
+
+  case ERROR:
+    return NULL;
+
+  case RET_FALSE:
+    nbdkit_error ("%s: %s method returned unexpected code (3/false)",
+                  script, method);
+    errno = EIO;
+    return NULL;
+
+  default: abort ();
+  }
+}
+
 void *
 sh_open (int readonly)
 {
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 374888a4..4fcc2a5a 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -301,6 +301,7 @@ static struct nbdkit_plugin plugin = {

   .preconnect        = sh_preconnect,
   .list_exports      = sh_list_exports,
+  .default_export    = sh_default_export,
   .open              = sh_open,
   .close             = sh_close,

diff --git a/plugins/sh/example.sh b/plugins/sh/example.sh
index 4f547db0..e0e1b773 100755
--- a/plugins/sh/example.sh
+++ b/plugins/sh/example.sh
@@ -85,7 +85,7 @@ case "$1" in
         echo parallel
         ;;

-    list_exports)
+    list_exports | default_export)
         # The following lists the names of all files in the current
         # directory that do not contain whitespace, backslash, or single
         # quotes.  No description accompanies the export names.
diff --git a/tests/test-eval-exports.sh b/tests/test-eval-exports.sh
index 7c946378..14b4c2ce 100755
--- a/tests/test-eval-exports.sh
+++ b/tests/test-eval-exports.sh
@@ -37,8 +37,8 @@ source ./functions.sh
 set -e
 set -x

-# requires nbdinfo --version # nbdinfo 1.3.9 was broken, so check this instead:
-requires nbdkit -U - memory 1 --run 'nbdinfo --size --json
"$uri"'
+requires nbdinfo --version
+requires nbdsh -c 'print (h.set_full_info)'
 requires jq --version

 sock=$(mktemp -u)
@@ -57,20 +57,19 @@ diff -u <(jq -c \
           '[.exports[] | [."export-name", .description,
."export-size"]]' \
           eval-exports.out) <(printf %s\\n
'[["",null,0]]')

-# Start a long-running server with .list_exports set to varying contents
+# Start a long-running server with .list_exports and .default_export
+# set to varying contents
 start_nbdkit -P eval-exports.pid -U $sock eval get_size='echo
"$2"|wc -c' \
-    open='echo "$3"' list_exports="cat
'$PWD/eval-exports.list'"
+    open='echo "$3"' list_exports="cat
'$PWD/eval-exports.list'" \
+    default_export="cat '$PWD/eval-exports.list'"

 # do_nbdkit EXPNAME EXPOUT
 do_nbdkit ()
 {
     # Check how the default export name is handled
-    # nbdinfo currently makes multiple connections, so we can't use the
-    # long-running server for validating default export name.
-    # XXX FIXME: requires .default_export in eval
-    : || nbdkit -U - -v eval list_exports="cat
'$PWD/eval-exports.list'" \
-      open='[ "$3" = "'"$1"'" ] || {
echo EINVAL wrong export >&2; exit 1; }' \
-      get_size='echo 0' --run 'nbdsh -u "$uri" -c
"exit()"'
+    nbdinfo --no-content nbd+unix://\?socket=$sock >eval-exports.out
+    diff -u <(sed -n 's/export="\(.*\)":/\1/p'
eval-exports.out) \
+         <(printf %s\\n "$1")
     # Check what exports are listed
     nbdinfo --list --json nbd+unix://\?socket=$sock >eval-exports.out
     cat eval-exports.out
@@ -79,9 +78,21 @@ do_nbdkit ()
               eval-exports.out) <(printf %s\\n "$2")
 }

-# With no file, .list_exports fails, but connecting works
+# With no file, .list_exports and .default_export both fail, preventing
+# connection to the default export, but not other exports
 nbdinfo --list --json nbd+unix://\?socket=$sock && fail=1
-nbdsh -u nbd+unix://\?socket=$sock -c 'quit()'
+nbdsh -u nbd+unix://\?socket=$sock -c 'quit()' && fail=1
+nbdsh -u nbd+unix:///name\?socket=$sock -c 'quit()'
+
+# Setting .default_export but not .list_exports advertises the canonical name
+nbdkit -U - eval default_export='echo hello' get_size='echo 0'
\
+       --run 'nbdinfo --list "$uri"' >eval-exports.out
+diff -u <(grep '^export=' eval-exports.out) <(echo
'export="hello":')
+
+# Failing .default_export without .list_exports results in an empty list
+nbdkit -U - eval default_export='echo ENOENT >&2; exit 1'
get_size='echo 0' \
+       --run 'nbdinfo --list "$uri"' >eval-exports.out
+diff -u <(grep '^export=' eval-exports.out) /dev/null

 # Various spellings of empty lists, producing 0 exports
 for fmt in '' 'NAMES\n' 'INTERLEAVED\n'
'NAMES+DESCRIPTIONS\n'; do
diff --git a/tests/test-tls-fallback.sh b/tests/test-tls-fallback.sh
index cf54a606..94449f31 100755
--- a/tests/test-tls-fallback.sh
+++ b/tests/test-tls-fallback.sh
@@ -60,12 +60,15 @@ cleanup_fn rm -f $files
 # Run dual-mode server
 start_nbdkit -P $pid -U $sock --tls=on --tls-psk=keys.psk \
     --filter=tls-fallback sh - tlsreadme=$'dummy\n' <<\EOF
+check () {
+ if test "$1" != true; then
+   echo 'EINVAL unexpected tls mode' 2>&1; exit 1
+ fi
+}
 case $1 in
   list_exports) echo NAMES; echo hello; echo world ;;
-  open) if test "$4" != true; then
-      echo 'EINVAL unexpected tls mode' 2>&1; exit 1
-    fi
-    echo $3 ;;
+  default_export) check "$3"; echo hello ;;
+  open) check "$4"; echo $3 ;;
   get_size) echo "$2" | wc -c ;;
   pread) echo "$2" | dd skip=$4 count=$3 iflag=skip_bytes,count_bytes
;;
   can_write | can_trim) exit 0 ;;
-- 
2.28.0

On Tue, Aug 25, 2020 at 10:46:57AM -0500, Eric Blake
wrote:
> + const char *nbdkit_string_intern (const char *str);
> +
> +Returns a copy of str, so that the caller may reclaim str and use the
> +copy in its place.  If the copy is created outside the scope of a
> +connection (such as during C<.load>), the lifetime of the copy will
> +last at least through C<.unload>; if it was created within a
> +connection (such as during C<.preconnect>, C<.default_export>,
or
> +C<.open>), the lifetime will last at least through C<.close>.
Interesting - this isn't exactly how I imagined this would work.
Would it make sense to intern strings for the lifetime of the server
no matter where they are allocated from?

Another suggestion would be for the function to take a flag indicating
preferred lifetime, but that presents additional complications for the
caller.

Also I imagined that this function would be a true (LISP-style)
INTERN, in that it would return a single pointer if called twice with
the two equal strings.  But I see from the implementation later on
that this isn't the case.
> -      keys[optind] = strndup (argv[optind], n);
> -      if (keys[optind] == NULL) {
> -        perror ("strndup");
> +      CLEANUP_FREE char *key = strndup (argv[optind], n);
> +      const char *safekey = nbdkit_string_intern (key);
> +      if (safekey == NULL)
>          exit (EXIT_FAILURE);
It seems it might also be nice to have a "strndup" version of the
intern function.

I'm not completely sure which of these behaviours is better.  Ideally
back when I started writing nbdkit I would have used a pool allocator,
but that's all water under the bridge now.  So none of the above
indicates I have a preference for a particular implementation, just
ideas for further discussion.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On Tue, Aug 25, 2020 at 10:46:58AM -0500, Eric Blake
wrote:
> Instead of needing to provide .unload, we can let nbdkit manage the
> lifetime for us.  However, if we like this approach, it may be wiser
> to introduce new variants of nbdkit_realpath/absolute_path which
> return const char * already intern'd.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> See the cover letter: if we like this, there are several other plugins
> that could also reduce their .unload, or maybe we want to add
> convenience functions to make the combo
> 'nbdkit_realpath/nbdkit_string_intern' simpler.
> 
>  plugins/file/file.c | 20 ++++++--------------
>  1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/plugins/file/file.c b/plugins/file/file.c
> index 08418194..6a0aad93 100644
> --- a/plugins/file/file.c
> +++ b/plugins/file/file.c
> @@ -64,8 +64,8 @@
>  #define fdatasync fsync
>  #endif
> 
> -static char *filename = NULL;
> -static char *directory = NULL;
> +static const char *filename = NULL;
> +static const char *directory = NULL;
> 
>  /* posix_fadvise mode: -1 = don't set it, or POSIX_FADV_*. */
>  static int fadvise_mode > @@ -91,13 +91,6 @@ is_enotsup (int err)
>    return err == ENOTSUP || err == EOPNOTSUPP;
>  }
> 
> -static void
> -file_unload (void)
> -{
> -  free (filename);
> -  free (directory);
> -}
> -
>  /* Called for each key=value passed on the command line.  This plugin
>   * only accepts file=<filename> and dir=<dirname>, where
exactly
>   * one is required.
> @@ -111,15 +104,15 @@ file_config (const char *key, const char *value)
>     * existence checks to the last possible moment.
>     */
>    if (strcmp (key, "file") == 0) {
> -    free (filename);
> -    filename = nbdkit_realpath (value);
> +    CLEANUP_FREE char *tmp = nbdkit_realpath (value);
> +    filename = nbdkit_string_intern (tmp);
>      if (!filename)
>        return -1;
>    }
>    else if (strcmp (key, "directory") == 0 ||
>             strcmp (key, "dir") == 0) {
> -    free (directory);
> -    directory = nbdkit_realpath (value);
> +    CLEANUP_FREE char *tmp = nbdkit_realpath (value);
> +    directory = nbdkit_string_intern (value);
>      if (!directory)
>        return -1;
>    }
> @@ -812,7 +805,6 @@ static struct nbdkit_plugin plugin = {
>    .name              = "file",
>    .longname          = "nbdkit file plugin",
>    .version           = PACKAGE_VERSION,
> -  .unload            = file_unload,
>    .config            = file_config,
>    .config_complete   = file_config_complete,
>    .config_help       = file_config_help,
> -- 
While this is kind of nice, it also IMHO demonstrates why having the
intern function do two different things implicitly has the potential
to cause confusion for users.  A developer might believe they could
copy the pattern
> +    CLEANUP_FREE char *tmp = nbdkit_realpath (value);
> +    directory = nbdkit_string_intern (value);
into a connected function, but would find that the string gets freed
at close rather than unload.

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

I think patches 1, 2 and 5 are fine.  It might be an idea to update
the ASCII art in docs/nbdkit-plugin.pod in patch 1.

Patches 3 and 4 are OK if you're happy with them but I have
reservations about the lifetime and design of nbdkit_string_intern and
maybe we should talk about that a bit more.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 8/25/20 10:46 AM, Eric Blake wrote:
> I'm about to add an 'exportname' filter, and in the process, I
noticed
> a few shortcomings in our API.  Time to fix those before the 1.22
> release locks our API in stone.  Overloading .list_exports in order to
> determine a canonical export name at .open time is awkward; the two
> uses (answering NBD_OPT_LIST for a full list, vs. remapping a client's
> "" into a canonical name during .open) are orthogonal enough to
> warrant separate plugin callbacks.  This will also make it easier to
> express the notion of no default export (connecting to "" is an
error)
> at the same time as listing other exports.  Another consideration is
> that when tls=1, the choice of export to expose pre-TLS vs. post-TLS
> may differ, but without a call to .open yet, our just-added
> nbdkit_is_tls() does not fit our preferred lifecycle, so this has to
> be a parameter to the new .default_export.  We will alter the
> signature of .list_exports soon; in the meantime, the bool
> default_only parameter is now ignored.
> 
> +++ b/server/plugins.c
> @@ -282,15 +283,29 @@ static int
>   plugin_list_exports (struct backend *b, int readonly, int default_only,
>                        struct nbdkit_exports *exports)
>   {
> -  GET_CONN;
>     struct backend_plugin *p = container_of (b, struct backend_plugin,
backend);
> 
> -  if (!p->plugin.list_exports)
> -    return nbdkit_add_export (exports, "", NULL);
> +  if (!p->plugin.list_exports) {
> +    const char *def = backend_default_export (b, readonly);
> +    if (def == NULL)
> +      return 0;
> +    return nbdkit_add_export (exports, def, NULL);
> +  }
This makes sense, but it is hard to replicate in language plugins.  As 
submitted in this round of patches, patch 5/5 causes test-eflags.sh to 
fail, because the sh plugin can't access the caching 
backend_default_export(), but instead called sh_default_export(), but 
the test was checking for proper caching.

My solution is to add nbdkit_add_default(exports), which tells nbdkit to 
append .default_export to the list at a later point in the process. 
I'll post v2 shortly.

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