Libguestfs - Jul 2020 - Re: [PATCH nbdkit] server: Pass the export name through filter .open calls.

To allow filters to modify the export name as it passes through the
layers this commit makes several changes:

The filter .open callback now takes an extra parameter, the export
name.  This is always non-NULL (for oldstyle it is "").  This string
has a short lifetime and filters that need to hang on to it must take
a copy.  The filter must pass the exportname parameter down to the
next layer (or potentially modify it).

The plugin .open callback is unchanged, but the binding to it in
server/plugins.c takes a copy of the exportname in the handle and this
is what nbdkit_export_name returns to plugins.  Eventually we will
deprecate that function and the V3 .open callback in plugins will have
the extra parameter, but this is not implemented yet.

nbdkit_export_name can only be called from plugins, not filters.
Filters should use the .open(...exportname) parameter if they need the
export name.

Filter .reopen also takes the exportname.  The only filter that uses
it (retry) must save the exportname from .open in order to pass it to
.reopen.  This filter already does the same for the readonly flag so
this seems reasonable.

The handling of NBD_OPT_SET_META_CONTEXT interacted with the export
name (see commit 20b8509a9ccdab118ce7b7043be63bbad74f1e79).  I have
attempted to keep previous behaviour in this change, but note that
there is no regression test for this.  I added a debug message so we
can tell when this unusual case actually happens which should help
with diagnosis of problems.

All of the above is internal refactoring and should have no visible
external effect on server behaviour or how plugins are written.
Filters need to be updated as discussed above.  All the in-tree
filters have been, and we assume there are no out of tree filters
because of the unstable filter API.

See also this thread:
https://www.redhat.com/archives/libguestfs/2020-July/thread.html#00087
---
 include/nbdkit-common.h              |  1 -
 include/nbdkit-filter.h              |  7 +--
 include/nbdkit-plugin.h              |  1 +
 server/internal.h                    | 26 +++++++----
 server/backend.c                     | 14 +++---
 server/connections.c                 |  2 +
 server/filters.c                     |  6 +--
 server/plugins.c                     | 20 ++++++++-
 server/protocol-handshake-newstyle.c | 65 ++++++++++++++++++----------
 server/protocol-handshake-oldstyle.c |  2 +-
 server/protocol-handshake.c          |  5 ++-
 server/public.c                      |  3 ++
 filters/cow/cow.c                    |  5 ++-
 filters/exitlast/exitlast.c          |  4 +-
 filters/ext2/ext2.c                  | 31 ++++++++++---
 filters/gzip/gzip.c                  |  5 ++-
 filters/limit/limit.c                |  4 +-
 filters/log/log.c                    |  5 ++-
 filters/partition/partition.c        |  5 ++-
 filters/rate/rate.c                  |  5 ++-
 filters/retry/retry.c                | 15 +++++--
 filters/tar/tar.c                    |  5 ++-
 filters/truncate/truncate.c          |  5 ++-
 filters/xz/xz.c                      |  5 ++-
 tests/test-layers-filter.c           |  5 ++-
 25 files changed, 171 insertions(+), 80 deletions(-)

diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 47288050..671cd4a4 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -110,7 +110,6 @@ extern int nbdkit_stdio_safe (void);
 extern int nbdkit_read_password (const char *value, char **password);
 extern char *nbdkit_realpath (const char *path);
 extern int nbdkit_nanosleep (unsigned sec, unsigned nsec);
-extern const char *nbdkit_export_name (void);
 extern int nbdkit_peer_name (struct sockaddr *addr, socklen_t *addrlen);
 extern void nbdkit_shutdown (void);
 
diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h
index 229a54b4..cec12db7 100644
--- a/include/nbdkit-filter.h
+++ b/include/nbdkit-filter.h
@@ -65,13 +65,14 @@ typedef int nbdkit_next_config_complete (nbdkit_backend
*nxdata);
 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_open (nbdkit_backend *nxdata, int readonly);
+typedef int nbdkit_next_open (nbdkit_backend *nxdata,
+                              int readonly, const char *exportname);
 
 struct nbdkit_next_ops {
   /* Performs close + open on the underlying chain.
    * Used by the retry filter.
    */
-  int (*reopen) (nbdkit_backend *nxdata, int readonly);
+  int (*reopen) (nbdkit_backend *nxdata, int readonly, const char *exportname);
 
   /* The rest of the next ops are the same as normal plugin operations. */
   int64_t (*get_size) (nbdkit_backend *nxdata);
@@ -156,7 +157,7 @@ struct nbdkit_filter {
                      int readonly);
 
   void * (*open) (nbdkit_next_open *next, nbdkit_backend *nxdata,
-                  int readonly);
+                  int readonly, const char *exportname);
   void (*close) (void *handle);
 
   int (*prepare) (struct nbdkit_next_ops *next_ops, nbdkit_backend *nxdata,
diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h
index cc261635..c6c1256c 100644
--- a/include/nbdkit-plugin.h
+++ b/include/nbdkit-plugin.h
@@ -142,6 +142,7 @@ struct nbdkit_plugin {
 };
 
 extern void nbdkit_set_error (int err);
+extern const char *nbdkit_export_name (void);
 
 #define NBDKIT_REGISTER_PLUGIN(plugin)                                  \
   NBDKIT_CXX_LANG_C                                                     \
diff --git a/server/internal.h b/server/internal.h
index ebc63a52..4911a49b 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -246,8 +246,6 @@ struct connection {
   struct handle *handles;       /* One per plugin and filter. */
   size_t nr_handles;
 
-  char exportname[NBD_MAX_STRING + 1];
-  uint32_t exportnamelen;
   uint32_t cflags;
   uint16_t eflags;
   bool handshake_complete;
@@ -255,6 +253,9 @@ struct connection {
   bool structured_replies;
   bool meta_context_base_allocation;
 
+  char *exportname_from_set_meta_context;
+  char *exportname;
+
   int sockin, sockout;
   connection_recv_function recv;
   connection_send_function send;
@@ -273,8 +274,9 @@ extern int connection_set_status (int value);
 
 /* protocol-handshake.c */
 extern int protocol_handshake (void);
-extern int protocol_common_open (uint64_t *exportsize, uint16_t *flags)
-  __attribute__((__nonnull__ (1, 2)));
+extern int protocol_common_open (uint64_t *exportsize, uint16_t *flags,
+                                 const char *exportname)
+  __attribute__((__nonnull__ (1, 2, 3)));
 
 /* protocol-handshake-oldstyle.c */
 extern int protocol_handshake_oldstyle (void);
@@ -358,7 +360,7 @@ struct backend {
   void (*get_ready) (struct backend *);
   void (*after_fork) (struct backend *);
   int (*preconnect) (struct backend *, int readonly);
-  void *(*open) (struct backend *, int readonly);
+  void *(*open) (struct backend *, int readonly, const char *exportname);
   int (*prepare) (struct backend *, void *handle, int readonly);
   int (*finalize) (struct backend *, void *handle);
   void (*close) (struct backend *, void *handle);
@@ -402,8 +404,13 @@ extern void backend_load (struct backend *b, const char
*name,
 extern void backend_unload (struct backend *b, void (*unload) (void))
   __attribute__((__nonnull__ (1)));
 
-extern int backend_open (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.
+ */
+extern int backend_open (struct backend *b,
+                         int readonly, const char *exportname)
+  __attribute__((__nonnull__ (1, 3)));
 extern int backend_prepare (struct backend *b)
   __attribute__((__nonnull__ (1)));
 extern int backend_finalize (struct backend *b)
@@ -414,8 +421,9 @@ extern bool backend_valid_range (struct backend *b,
                                  uint64_t offset, uint32_t count)
   __attribute__((__nonnull__ (1)));
 
-extern int backend_reopen (struct backend *b, int readonly)
-  __attribute__((__nonnull__ (1)));
+extern int backend_reopen (struct backend *b,
+                           int readonly, const char *exportname)
+  __attribute__((__nonnull__ (1, 3)));
 extern int64_t backend_get_size (struct backend *b)
   __attribute__((__nonnull__ (1)));
 extern int backend_can_write (struct backend *b)
diff --git a/server/backend.c b/server/backend.c
index cf87e35a..d39fdeaf 100644
--- a/server/backend.c
+++ b/server/backend.c
@@ -152,12 +152,13 @@ backend_unload (struct backend *b, void (*unload) (void))
 }
 
 int
-backend_open (struct backend *b, int readonly)
+backend_open (struct backend *b, int readonly, const char *exportname)
 {
   GET_CONN;
   struct handle *h = get_handle (conn, b->i);
 
-  controlpath_debug ("%s: open readonly=%d", b->name, readonly);
+  controlpath_debug ("%s: open readonly=%d
exportname=\"%s\"",
+                     b->name, readonly, exportname);
 
   assert (h->handle == NULL);
   assert ((h->state & HANDLE_OPEN) == 0);
@@ -168,7 +169,7 @@ backend_open (struct backend *b, int readonly)
   /* Most filters will call next_open first, resulting in
    * inner-to-outer ordering.
    */
-  h->handle = b->open (b, readonly);
+  h->handle = b->open (b, readonly, exportname);
   controlpath_debug ("%s: open returned handle %p", b->name,
h->handle);
 
   if (h->handle == NULL) {
@@ -268,14 +269,15 @@ backend_valid_range (struct backend *b, uint64_t offset,
uint32_t count)
 /* Wrappers for all callbacks in a filter's struct nbdkit_next_ops. */
 
 int
-backend_reopen (struct backend *b, int readonly)
+backend_reopen (struct backend *b, int readonly, const char *exportname)
 {
-  controlpath_debug ("%s: reopen readonly=%d", b->name, readonly);
+  controlpath_debug ("%s: reopen readonly=%d
exportname=\"%s\"",
+                     b->name, readonly, exportname);
 
   if (backend_finalize (b) == -1)
     return -1;
   backend_close (b);
-  if (backend_open (b, readonly) == -1) {
+  if (backend_open (b, readonly, exportname) == -1) {
     backend_close (b);
     return -1;
   }
diff --git a/server/connections.c b/server/connections.c
index cc552b90..69e4c0c0 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -356,6 +356,8 @@ free_connection (struct connection *conn)
   pthread_mutex_destroy (&conn->write_lock);
   pthread_mutex_destroy (&conn->status_lock);
 
+  free (conn->exportname_from_set_meta_context);
+  free (conn->exportname);
   free (conn->handles);
   free (conn);
   threadlocal_set_conn (NULL);
diff --git a/server/filters.c b/server/filters.c
index 2d705e1e..7d268096 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -238,7 +238,7 @@ plugin_magic_config_key (struct backend *b)
 }
 
 static void *
-filter_open (struct backend *b, int readonly)
+filter_open (struct backend *b, int readonly, const char *exportname)
 {
   struct backend_filter *f = container_of (b, struct backend_filter, backend);
   void *handle;
@@ -247,8 +247,8 @@ filter_open (struct backend *b, int readonly)
    * inner-to-outer ordering.
    */
   if (f->filter.open)
-    handle = f->filter.open (backend_open, b->next, readonly);
-  else if (backend_open (b->next, readonly) == -1)
+    handle = f->filter.open (backend_open, b->next, readonly,
exportname);
+  else if (backend_open (b->next, readonly, exportname) == -1)
     handle = NULL;
   else
     handle = NBDKIT_HANDLE_NOT_NEEDED;
diff --git a/server/plugins.c b/server/plugins.c
index 285569bb..8449b1d9 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -278,12 +278,30 @@ plugin_preconnect (struct backend *b, int readonly)
 }
 
 static void *
-plugin_open (struct backend *b, int readonly)
+plugin_open (struct backend *b, int readonly, const char *exportname)
 {
+  GET_CONN;
   struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
 
   assert (p->plugin.open != NULL);
 
+  /* Save the exportname since the lifetime of the string passed in
+   * here is likely to be brief.  In addition this provides a place
+   * for nbdkit_export_name to retrieve it if called from the plugin.
+   *
+   * In API V3 we propose to pass the exportname as an extra parameter
+   * to the (new) plugin.open and deprecate nbdkit_export_name for V3
+   * users.  Even then we will still need to save it in the handle
+   * because of the lifetime issue.
+   */
+  if (conn->exportname == NULL) {
+    conn->exportname = strdup (exportname);
+    if (conn->exportname == NULL) {
+      nbdkit_error ("strdup: %m");
+      return NULL;
+    }
+  }
+
   return p->plugin.open (readonly);
 }
 
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index fa44f253..0ed3c7c7 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char *fmt,
...)
  * in that function, and must not cause any wire traffic.
  */
 static int
-finish_newstyle_options (uint64_t *exportsize)
+finish_newstyle_options (uint64_t *exportsize,
+                         const char *exportname_in, uint32_t exportnamelen)
 {
   GET_CONN;
 
-  if (protocol_common_open (exportsize, &conn->eflags) == -1)
+  /* Since the exportname string passed here comes directly out of the
+   * NBD protocol make a temporary copy of the exportname into a
+   * \0-terminated buffer.
+   */
+  CLEANUP_FREE char *exportname = strndup (exportname_in, exportnamelen);
+  if (exportname == NULL) {
+    nbdkit_error ("strndup: %m");
+    return -1;
+  }
+
+  if (conn->exportname_from_set_meta_context &&
+      strcmp (conn->exportname_from_set_meta_context, exportname) != 0) {
+    debug ("newstyle negotiation: NBD_OPT_SET_META_CONTEXT export name
\"%s\" ≠ final client exportname \"%s\", so discarding the
previous context",
+           conn->exportname_from_set_meta_context, exportname);
+    conn->meta_context_base_allocation = false;
+  }
+
+  if (protocol_common_open (exportsize, &conn->eflags, exportname) ==
-1)
     return -1;
 
   debug ("newstyle negotiation: flags: export 0x%x",
conn->eflags);
@@ -238,22 +256,13 @@ check_string (uint32_t option, char *buf, uint32_t len,
uint32_t maxlen,
  */
 static int
 check_export_name (uint32_t option, char *buf,
-                   uint32_t exportnamelen, uint32_t maxlen, bool save)
+                   uint32_t exportnamelen, uint32_t maxlen)
 {
   GET_CONN;
 
   if (check_string (option, buf, exportnamelen, maxlen, "export
name") == -1)
     return -1;
 
-  assert (exportnamelen < sizeof conn->exportname);
-  if (save) {
-    if (exportnamelen != conn->exportnamelen ||
-        memcmp (conn->exportname, buf, exportnamelen) != 0)
-      conn->meta_context_base_allocation = false;
-    memcpy (conn->exportname, buf, exportnamelen);
-    conn->exportname[exportnamelen] = '\0';
-    conn->exportnamelen = exportnamelen;
-  }
   debug ("newstyle negotiation: %s: client requested export
'%.*s'",
          name_of_nbd_opt (option), (int) exportnamelen, buf);
   return 0;
@@ -329,13 +338,13 @@ negotiate_handshake_newstyle_options (void)
       if (conn_recv_full (data, optlen,
                           "read: %s: %m", name_of_nbd_opt (option))
== -1)
         return -1;
-      if (check_export_name (option, data, optlen, optlen, true) == -1)
+      if (check_export_name (option, data, optlen, optlen) == -1)
         return -1;
 
       /* We have to finish the handshake by sending handshake_finish.
        * On failure, we have to disconnect.
        */
-      if (finish_newstyle_options (&exportsize) == -1)
+      if (finish_newstyle_options (&exportsize, data, optlen) == -1)
         return -1;
 
       memset (&handshake_finish, 0, sizeof handshake_finish);
@@ -468,7 +477,7 @@ negotiate_handshake_newstyle_options (void)
          * or else we drop the support for that context.
          */
         if (check_export_name (option, &data[4], exportnamelen,
-                               optlen - 6, true) == -1) {
+                               optlen - 6) == -1) {
           if (send_newstyle_option_reply (option, NBD_REP_ERR_INVALID)
               == -1)
             return -1;
@@ -483,7 +492,8 @@ negotiate_handshake_newstyle_options (void)
          * client and let them try another NBD_OPT, rather than
          * disconnecting.
          */
-        if (finish_newstyle_options (&exportsize) == -1) {
+        if (finish_newstyle_options (&exportsize,
+                                     &data[4], exportnamelen) == -1) {
           if (backend_finalize (top) == -1)
             return -1;
           backend_close (top);
@@ -601,17 +611,26 @@ negotiate_handshake_newstyle_options (void)
           continue;
         }
 
+        memcpy (&exportnamelen, &data[0], 4);
+        exportnamelen = be32toh (exportnamelen);
+        what = "validating export name";
+        if (check_export_name (option, &data[4], exportnamelen,
+                               optlen - 8) == -1)
+          goto opt_meta_invalid_option_len;
+
         /* Remember the export name: the NBD spec says that if the client
          * later uses NBD_OPT_GO on a different export, then the context
          * returned here is not usable.
          */
-        memcpy (&exportnamelen, &data[0], 4);
-        exportnamelen = be32toh (exportnamelen);
-        what = "validating export name";
-        if (check_export_name (option, &data[4], exportnamelen,
-                               optlen - 8,
-                               option == NBD_OPT_SET_META_CONTEXT) == -1)
-          goto opt_meta_invalid_option_len;
+        if (option == NBD_OPT_SET_META_CONTEXT) {
+          conn->exportname_from_set_meta_context +            strndup
(&data[4], exportnamelen);
+          if (conn->exportname_from_set_meta_context == NULL) {
+            nbdkit_error ("malloc: %m");
+            return -1;
+          }
+        }
+
         opt_index = 4 + exportnamelen;
 
         /* Read the number of queries. */
diff --git a/server/protocol-handshake-oldstyle.c
b/server/protocol-handshake-oldstyle.c
index 4ad4720f..d4692362 100644
--- a/server/protocol-handshake-oldstyle.c
+++ b/server/protocol-handshake-oldstyle.c
@@ -56,7 +56,7 @@ protocol_handshake_oldstyle (void)
   /* With oldstyle, our only option if .open or friends fail is to
    * disconnect, as we cannot report the problem to the client.
    */
-  if (protocol_common_open (&exportsize, &eflags) == -1)
+  if (protocol_common_open (&exportsize, &eflags, "") == -1)
     return -1;
 
   gflags = 0;
diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c
index 5e4b5e6e..80233713 100644
--- a/server/protocol-handshake.c
+++ b/server/protocol-handshake.c
@@ -72,14 +72,15 @@ protocol_handshake ()
  * simply opening a TCP connection.
  */
 int
-protocol_common_open (uint64_t *exportsize, uint16_t *flags)
+protocol_common_open (uint64_t *exportsize, uint16_t *flags,
+                      const char *exportname)
 {
   GET_CONN;
   int64_t size;
   uint16_t eflags = NBD_FLAG_HAS_FLAGS;
   int fl;
 
-  if (backend_open (top, read_only) == -1)
+  if (backend_open (top, read_only, exportname) == -1)
     return -1;
 
   /* Prepare (for filters), called just after open. */
diff --git a/server/public.c b/server/public.c
index cef25426..c00a5cb6 100644
--- a/server/public.c
+++ b/server/public.c
@@ -654,6 +654,9 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
 #endif
 }
 
+/* This function will be deprecated for API V3 users.  The preferred
+ * approach will be to get the exportname from .open().
+ */
 const char *
 nbdkit_export_name (void)
 {
diff --git a/filters/cow/cow.c b/filters/cow/cow.c
index fae36174..0faf2726 100644
--- a/filters/cow/cow.c
+++ b/filters/cow/cow.c
@@ -93,10 +93,11 @@ cow_config (nbdkit_next_config *next, void *nxdata,
   "cow-on-cache=<BOOL>  Set to true to treat client cache requests
as writes.\n"
 
 static void *
-cow_open (nbdkit_next_open *next, void *nxdata, int readonly)
+cow_open (nbdkit_next_open *next, void *nxdata,
+          int readonly, const char *exportname)
 {
   /* Always pass readonly=1 to the underlying plugin. */
-  if (next (nxdata, 1) == -1)
+  if (next (nxdata, 1, exportname) == -1)
     return NULL;
 
   return NBDKIT_HANDLE_NOT_NEEDED;
diff --git a/filters/exitlast/exitlast.c b/filters/exitlast/exitlast.c
index 51efda03..378cda75 100644
--- a/filters/exitlast/exitlast.c
+++ b/filters/exitlast/exitlast.c
@@ -51,9 +51,9 @@ static _Atomic unsigned connections;
 
 static void *
 exitlast_open (nbdkit_next_open *next, nbdkit_backend *nxdata,
-               int readonly)
+               int readonly, const char *exportname)
 {
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   connections++;
diff --git a/filters/ext2/ext2.c b/filters/ext2/ext2.c
index 92c8601a..72b7ac9f 100644
--- a/filters/ext2/ext2.c
+++ b/filters/ext2/ext2.c
@@ -112,6 +112,7 @@ ext2_config_complete (nbdkit_next_config_complete *next,
void *nxdata)
 
 /* The per-connection handle. */
 struct handle {
+  char *exportname;             /* Client export name. */
   ext2_filsys fs;               /* Filesystem handle. */
   ext2_ino_t ino;               /* Inode of open file. */
   ext2_file_t file;             /* File handle. */
@@ -120,20 +121,37 @@ struct handle {
 
 /* Create the per-connection handle. */
 static void *
-ext2_open (nbdkit_next_open *next, void *nxdata, int readonly)
+ext2_open (nbdkit_next_open *next, void *nxdata,
+           int readonly, const char *exportname)
 {
   struct handle *h;
 
-  /* Request write access to the underlying plugin, for journal replay. */
-  if (next (nxdata, 0) == -1)
-    return NULL;
-
   h = calloc (1, sizeof *h);
   if (h == NULL) {
     nbdkit_error ("calloc: %m");
     return NULL;
   }
 
+  /* Save the client exportname in the handle. */
+  h->exportname = strdup (exportname);
+  if (h->exportname == NULL) {
+    nbdkit_error ("strdup: %m");
+    free (h);
+    return NULL;
+  }
+
+  /* If file == NULL (ie. using exportname) then don't
+   * pass the client exportname to the lower layers.
+   */
+  exportname = file ? exportname : "";
+
+  /* Request write access to the underlying plugin, for journal replay. */
+  if (next (nxdata, 0, exportname) == -1) {
+    free (h->exportname);
+    free (h);
+    return NULL;
+  }
+
   return h;
 }
 
@@ -148,7 +166,7 @@ ext2_prepare (struct nbdkit_next_ops *next_ops, void
*nxdata, void *handle,
   struct ext2_inode inode;
   int64_t r;
   CLEANUP_FREE char *name = NULL;
-  const char *fname = file ?: nbdkit_export_name ();
+  const char *fname = file ?: h->exportname;
   CLEANUP_FREE char *absname = NULL;
 
   fs_flags = 0;
@@ -242,6 +260,7 @@ ext2_close (void *handle)
     ext2fs_file_close (h->file);
     ext2fs_close (h->fs);
   }
+  free (h->exportname);
   free (h);
 }
 
diff --git a/filters/gzip/gzip.c b/filters/gzip/gzip.c
index 31fde3bb..8323b882 100644
--- a/filters/gzip/gzip.c
+++ b/filters/gzip/gzip.c
@@ -74,10 +74,11 @@ gzip_thread_model (void)
 }
 
 static void *
-gzip_open (nbdkit_next_open *next, nbdkit_backend *nxdata, int readonly)
+gzip_open (nbdkit_next_open *next, nbdkit_backend *nxdata,
+           int readonly, const char *exportname)
 {
   /* Always pass readonly=1 to the underlying plugin. */
-  if (next (nxdata, 1) == -1)
+  if (next (nxdata, 1, exportname) == -1)
     return NULL;
 
   return NBDKIT_HANDLE_NOT_NEEDED;
diff --git a/filters/limit/limit.c b/filters/limit/limit.c
index 563481fa..7c4477eb 100644
--- a/filters/limit/limit.c
+++ b/filters/limit/limit.c
@@ -91,9 +91,9 @@ limit_preconnect (nbdkit_next_preconnect *next, nbdkit_backend
*nxdata,
 
 static void *
 limit_open (nbdkit_next_open *next, nbdkit_backend *nxdata,
-            int readonly)
+            int readonly, const char *exportname)
 {
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   ACQUIRE_LOCK_FOR_CURRENT_SCOPE (&lock);
diff --git a/filters/log/log.c b/filters/log/log.c
index f47a6a93..8b146f1c 100644
--- a/filters/log/log.c
+++ b/filters/log/log.c
@@ -227,11 +227,12 @@ output_return (struct handle *h, const char *act, uint64_t
id, int r, int *err)
 
 /* Open a connection. */
 static void *
-log_open (nbdkit_next_open *next, void *nxdata, int readonly)
+log_open (nbdkit_next_open *next, void *nxdata,
+          int readonly, const char *exportname)
 {
   struct handle *h;
 
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   h = malloc (sizeof *h);
diff --git a/filters/partition/partition.c b/filters/partition/partition.c
index 41b85312..c348664b 100644
--- a/filters/partition/partition.c
+++ b/filters/partition/partition.c
@@ -87,11 +87,12 @@ struct handle {
 
 /* Open a connection. */
 static void *
-partition_open (nbdkit_next_open *next, void *nxdata, int readonly)
+partition_open (nbdkit_next_open *next, void *nxdata,
+                int readonly, const char *exportname)
 {
   struct handle *h;
 
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   h = malloc (sizeof *h);
diff --git a/filters/rate/rate.c b/filters/rate/rate.c
index 95b83024..6e99fcc7 100644
--- a/filters/rate/rate.c
+++ b/filters/rate/rate.c
@@ -162,11 +162,12 @@ rate_get_ready (nbdkit_next_get_ready *next, void *nxdata)
 
 /* Create the per-connection handle. */
 static void *
-rate_open (nbdkit_next_open *next, void *nxdata, int readonly)
+rate_open (nbdkit_next_open *next, void *nxdata,
+           int readonly, const char *exportname)
 {
   struct rate_handle *h;
 
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   h = malloc (sizeof *h);
diff --git a/filters/retry/retry.c b/filters/retry/retry.c
index 8037f383..be334c39 100644
--- a/filters/retry/retry.c
+++ b/filters/retry/retry.c
@@ -106,16 +106,18 @@ retry_config (nbdkit_next_config *next, void *nxdata,
 
 struct retry_handle {
   int readonly;                 /* Save original readonly setting. */
+  const char *exportname;       /* Client exportname. */
   unsigned reopens;
   bool open;
 };
 
 static void *
-retry_open (nbdkit_next_open *next, void *nxdata, int readonly)
+retry_open (nbdkit_next_open *next, void *nxdata,
+            int readonly, const char *exportname)
 {
   struct retry_handle *h;
 
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   h = malloc (sizeof *h);
@@ -125,6 +127,12 @@ retry_open (nbdkit_next_open *next, void *nxdata, int
readonly)
   }
 
   h->readonly = readonly;
+  h->exportname = strdup (exportname);
+  if (h->exportname == NULL) {
+    nbdkit_error ("strdup: %m");
+    free (h);
+    return NULL;
+  }
   h->reopens = 0;
   h->open = true;
 
@@ -198,7 +206,8 @@ do_retry (struct retry_handle *h,
 
   /* Reopen the connection. */
   h->reopens++;
-  if (next_ops->reopen (nxdata, h->readonly || force_readonly) == -1) {
+  if (next_ops->reopen (nxdata,
+                        h->readonly || force_readonly, h->exportname) ==
-1) {
     /* If the reopen fails we treat it the same way as a command
      * failing.
      */
diff --git a/filters/tar/tar.c b/filters/tar/tar.c
index a2b26935..c04f09dd 100644
--- a/filters/tar/tar.c
+++ b/filters/tar/tar.c
@@ -112,11 +112,12 @@ struct handle {
 };
 
 static void *
-tar_open (nbdkit_next_open *next, nbdkit_backend *nxdata, int readonly)
+tar_open (nbdkit_next_open *next, nbdkit_backend *nxdata,
+          int readonly, const char *exportname)
 {
   struct handle *h;
 
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   h = calloc (1, sizeof *h);
diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c
index 485f7f45..ee384871 100644
--- a/filters/truncate/truncate.c
+++ b/filters/truncate/truncate.c
@@ -124,11 +124,12 @@ struct handle {
 
 /* Open a connection. */
 static void *
-truncate_open (nbdkit_next_open *next, void *nxdata, int readonly)
+truncate_open (nbdkit_next_open *next, void *nxdata,
+               int readonly, const char *exportname)
 {
   struct handle *h;
 
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   h = malloc (sizeof *h); /* h is populated during .prepare */
diff --git a/filters/xz/xz.c b/filters/xz/xz.c
index 365958da..9154dbeb 100644
--- a/filters/xz/xz.c
+++ b/filters/xz/xz.c
@@ -89,12 +89,13 @@ struct xz_handle {
 
 /* Create the per-connection handle. */
 static void *
-xz_open (nbdkit_next_open *next, void *nxdata, int readonly)
+xz_open (nbdkit_next_open *next, void *nxdata,
+         int readonly, const char *exportname)
 {
   struct xz_handle *h;
 
   /* Always pass readonly=1 to the underlying plugin. */
-  if (next (nxdata, 1) == -1)
+  if (next (nxdata, 1, exportname) == -1)
     return NULL;
 
   h = malloc (sizeof *h);
diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c
index b6ca05bd..29d8b669 100644
--- a/tests/test-layers-filter.c
+++ b/tests/test-layers-filter.c
@@ -107,7 +107,8 @@ test_layers_filter_preconnect (nbdkit_next_preconnect *next,
 }
 
 static void *
-test_layers_filter_open (nbdkit_next_open *next, void *nxdata, int readonly)
+test_layers_filter_open (nbdkit_next_open *next, void *nxdata,
+                         int readonly, const char *exportname)
 {
   struct handle *h = malloc (sizeof *h);
 
@@ -118,7 +119,7 @@ test_layers_filter_open (nbdkit_next_open *next, void
*nxdata, int readonly)
   h->nxdata = nxdata;
   h->next_ops = NULL;
 
-  if (next (nxdata, readonly) == -1)
+  if (next (nxdata, readonly, exportname) == -1)
     return NULL;
 
   /* Debug after recursing, to show opposite order from .close */
-- 
2.27.0

On 7/21/20 3:47 PM, Richard W.M. Jones wrote:
> To allow filters to modify the export name as it passes through the
> layers this commit makes several changes:
> 
> The filter .open callback now takes an extra parameter, the export
> name.  This is always non-NULL (for oldstyle it is "").  This
string
> has a short lifetime and filters that need to hang on to it must take
> a copy.  The filter must pass the exportname parameter down to the
> next layer (or potentially modify it).
> 
> The plugin .open callback is unchanged, but the binding to it in
> server/plugins.c takes a copy of the exportname in the handle and this
> is what nbdkit_export_name returns to plugins.  Eventually we will
> deprecate that function and the V3 .open callback in plugins will have
> the extra parameter, but this is not implemented yet.
> 
> nbdkit_export_name can only be called from plugins, not filters.
> Filters should use the .open(...exportname) parameter if they need the
> export name.
> 
> Filter .reopen also takes the exportname.  The only filter that uses
> it (retry) must save the exportname from .open in order to pass it to
> .reopen.  This filter already does the same for the readonly flag so
> this seems reasonable.
> 
> The handling of NBD_OPT_SET_META_CONTEXT interacted with the export
> name (see commit 20b8509a9ccdab118ce7b7043be63bbad74f1e79).  I have
> attempted to keep previous behaviour in this change, but note that
> there is no regression test for this.  I added a debug message so we
> can tell when this unusual case actually happens which should help
> with diagnosis of problems.
Yeah, that commit specifically mentioned that I used gdb breakpoints in 
qemu-io to test things, and was not interested in hacking libnbd at the 
time.  Although now that we are debating about exposing nbd_opt_* 
commands in libnbd to someone that opts in, maybe that _could_ be a case 
to write such a regression test.  Meanwhile, I'll just try to fire up 
another gdb session to prove to myself that things still work.
> 
> All of the above is internal refactoring and should have no visible
> external effect on server behaviour or how plugins are written.
> Filters need to be updated as discussed above.  All the in-tree
> filters have been, and we assume there are no out of tree filters
> because of the unstable filter API.
And even if there are, whoever maintains it can get a good idea of what 
to do by reading this patch ;)
> 
> See also this thread:
> https://www.redhat.com/archives/libguestfs/2020-July/thread.html#00087
> ---
> +++ b/server/internal.h
> @@ -246,8 +246,6 @@ struct connection {
>     struct handle *handles;       /* One per plugin and filter. */
>     size_t nr_handles;
>   
> -  char exportname[NBD_MAX_STRING + 1];
> -  uint32_t exportnamelen;
>     uint32_t cflags;
>     uint16_t eflags;
>     bool handshake_complete;
> @@ -255,6 +253,9 @@ struct connection {
>     bool structured_replies;
>     bool meta_context_base_allocation;
>   
> +  char *exportname_from_set_meta_context;
> +  char *exportname;
Interesting switch from array to pointer, but it should work.
> +++ b/server/plugins.c
> @@ -278,12 +278,30 @@ plugin_preconnect (struct backend *b, int readonly)
>   }
>   
>   static void *
> -plugin_open (struct backend *b, int readonly)
> +plugin_open (struct backend *b, int readonly, const char *exportname)
>   {
> +  GET_CONN;
>     struct backend_plugin *p = container_of (b, struct backend_plugin,
backend);
>   
>     assert (p->plugin.open != NULL);
>   
> +  /* Save the exportname since the lifetime of the string passed in
> +   * here is likely to be brief.  In addition this provides a place
> +   * for nbdkit_export_name to retrieve it if called from the plugin.
> +   *
> +   * In API V3 we propose to pass the exportname as an extra parameter
> +   * to the (new) plugin.open and deprecate nbdkit_export_name for V3
> +   * users.  Even then we will still need to save it in the handle
> +   * because of the lifetime issue.
> +   */
> +  if (conn->exportname == NULL) {
Can't we assert(!conn->exportname) at this point?  After all, we only 
ever call .open at most once per connection.
> +++ b/server/protocol-handshake-newstyle.c
> @@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char
*fmt, ...)
>    * in that function, and must not cause any wire traffic.
>    */
>   static int
> -finish_newstyle_options (uint64_t *exportsize)
> +finish_newstyle_options (uint64_t *exportsize,
> +                         const char *exportname_in, uint32_t
exportnamelen)
>   {
>     GET_CONN;
>   
> -  if (protocol_common_open (exportsize, &conn->eflags) == -1)
> +  /* Since the exportname string passed here comes directly out of the
> +   * NBD protocol make a temporary copy of the exportname into a
> +   * \0-terminated buffer.
> +   */
> +  CLEANUP_FREE char *exportname = strndup (exportname_in, exportnamelen);
> +  if (exportname == NULL) {
> +    nbdkit_error ("strndup: %m");
> +    return -1;
> +  }
> +
> +  if (conn->exportname_from_set_meta_context &&
> +      strcmp (conn->exportname_from_set_meta_context, exportname) != 0)
{
> +    debug ("newstyle negotiation: NBD_OPT_SET_META_CONTEXT export
name \"%s\" ≠ final client exportname \"%s\", so discarding
the previous context",
Long line, and I don't know if we use UTF-8 in other debug messages or 
should stick to straight ascii.  Compilation may have a glitch if 
compiled under a different locale than the end binary runs in, but these 
days, it's uncommon to find someone running in a single-byte locale 
instead of UTF-8.
> +++ b/filters/ext2/ext2.c
>   /* Create the per-connection handle. */
>   static void *
> -ext2_open (nbdkit_next_open *next, void *nxdata, int readonly)
> +ext2_open (nbdkit_next_open *next, void *nxdata,
> +           int readonly, const char *exportname)
> +
> +  /* If file == NULL (ie. using exportname) then don't
> +   * pass the client exportname to the lower layers.
> +   */
> +  exportname = file ? exportname : "";
> +
> +  /* Request write access to the underlying plugin, for journal replay. */
> +  if (next (nxdata, 0, exportname) == -1) {
Nice demonstration of when the filter changes things vs. does 
pass-through, and will be easy enough to copy into the tar filter.
> +    free (h->exportname);
> +    free (h);
> +    return NULL;
> +  }
> +
>     return h;
>   }
>   
> @@ -148,7 +166,7 @@ ext2_prepare (struct nbdkit_next_ops *next_ops, void
*nxdata, void *handle,
>     struct ext2_inode inode;
>     int64_t r;
>     CLEANUP_FREE char *name = NULL;
> -  const char *fname = file ?: nbdkit_export_name ();
> +  const char *fname = file ?: h->exportname;
Hmm - we already pass the same 'readonly' state to filter's
.prepare,
but not to backend_prepare(), which has to reconstruct it.  Would it be 
easier to also change the signature of backend_prepare() to take both 
the original readonly and exportname passed to backend_open(), rather 
than making the filter have to save it off in the filter?  It looks like 
protocol-handshake.c is the only caller, and still has everything in 
scope at the time.

> +++ b/filters/log/log.c
> @@ -227,11 +227,12 @@ output_return (struct handle *h, const char *act,
uint64_t id, int r, int *err)
>   
>   /* Open a connection. */
>   static void *
> -log_open (nbdkit_next_open *next, void *nxdata, int readonly)
> +log_open (nbdkit_next_open *next, void *nxdata,
> +          int readonly, const char *exportname)
>   {
>     struct handle *h;
>   
> -  if (next (nxdata, readonly) == -1)
> +  if (next (nxdata, readonly, exportname) == -1)
>       return NULL;
Pre-existing - the log filter should include the exportname somewhere in 
its output log.  Well, nothing like the present to fix it ;)
> +++ b/filters/retry/retry.c
> @@ -106,16 +106,18 @@ retry_config (nbdkit_next_config *next, void *nxdata,
>   
>   struct retry_handle {
>     int readonly;                 /* Save original readonly setting. */
> +  const char *exportname;       /* Client exportname. */
>     unsigned reopens;
>     bool open;
>   };
>   
>   static void *
> -retry_open (nbdkit_next_open *next, void *nxdata, int readonly)
> +retry_open (nbdkit_next_open *next, void *nxdata,
> +            int readonly, const char *exportname)
>   {
>     struct retry_handle *h;
>   
> -  if (next (nxdata, readonly) == -1)
> +  if (next (nxdata, readonly, exportname) == -1)
>       return NULL;
>   
>     h = malloc (sizeof *h);
> @@ -125,6 +127,12 @@ retry_open (nbdkit_next_open *next, void *nxdata, int
readonly)
>     }
>   
>     h->readonly = readonly;
> +  h->exportname = strdup (exportname);
While I think we can avoid the strdup in the ext2 filter, I agree that 
the retry filter does have to store it.

Otherwise looking good.

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

On 7/21/20 7:41 PM, Eric Blake wrote:
>> @@ -148,7 +166,7 @@ ext2_prepare (struct nbdkit_next_ops *next_ops, 
>> void *nxdata, void *handle,
>>     struct ext2_inode inode;
>>     int64_t r;
>>     CLEANUP_FREE char *name = NULL;
>> -  const char *fname = file ?: nbdkit_export_name ();
>> +  const char *fname = file ?: h->exportname;
> 
> Hmm - we already pass the same 'readonly' state to filter's
.prepare,
> but not to backend_prepare(), which has to reconstruct it.  Would it be 
> easier to also change the signature of backend_prepare() to take both 
> the original readonly and exportname passed to backend_open(), rather 
> than making the filter have to save it off in the filter?  It looks like 
> protocol-handshake.c is the only caller, and still has everything in 
> scope at the time.
Nope, we can't.  It's because the sequence is:

backend_open("clientname")
   outer_filter.open("clientname")
     next_open("outername")
       inner_filter.open("outername")
         next_open("innername")
           plugin.open("innername")
backend_prepare()
   backend_prepare()
     backend_prepare()
       plugin.prepare()
     inner_filter.prepare()
   outer_filter.prepare()

while protocol-handshake.c has the same "clientname" to pass to both 
backend_open() and backend_prepare(), the outer-to-inner order of .open 
is (intentionally) opposite the inner-to-outer order of .prepare, and 
the name passed to plugin.open has indeed left scope.  Unless we want 
backend.c to start storing the name passed into backend_open().
> 
>> +++ b/filters/log/log.c
>> @@ -227,11 +227,12 @@ output_return (struct handle *h, const char 
>> *act, uint64_t id, int r, int *err)
>>   /* Open a connection. */
>>   static void *
>> -log_open (nbdkit_next_open *next, void *nxdata, int readonly)
>> +log_open (nbdkit_next_open *next, void *nxdata,
>> +          int readonly, const char *exportname)
>>   {
>>     struct handle *h;
>> -  if (next (nxdata, readonly) == -1)
>> +  if (next (nxdata, readonly, exportname) == -1)
>>       return NULL;
> 
> Pre-existing - the log filter should include the exportname somewhere in 
> its output log.  Well, nothing like the present to fix it ;)
I did that in the meantime, so you'll need a slight rebase.

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

On Tue, Jul 21, 2020 at 07:41:21PM -0500, Eric Blake
wrote:
> On 7/21/20 3:47 PM, Richard W.M. Jones wrote:
> >+++ b/server/plugins.c
> >@@ -278,12 +278,30 @@ plugin_preconnect (struct backend *b, int
readonly)
> >  }
> >  static void *
> >-plugin_open (struct backend *b, int readonly)
> >+plugin_open (struct backend *b, int readonly, const char *exportname)
> >  {
> >+  GET_CONN;
> >    struct backend_plugin *p = container_of (b, struct backend_plugin,
backend);
> >    assert (p->plugin.open != NULL);
> >+  /* Save the exportname since the lifetime of the string passed in
> >+   * here is likely to be brief.  In addition this provides a place
> >+   * for nbdkit_export_name to retrieve it if called from the plugin.
> >+   *
> >+   * In API V3 we propose to pass the exportname as an extra parameter
> >+   * to the (new) plugin.open and deprecate nbdkit_export_name for V3
> >+   * users.  Even then we will still need to save it in the handle
> >+   * because of the lifetime issue.
> >+   */
> >+  if (conn->exportname == NULL) {
> 
> Can't we assert(!conn->exportname) at this point?  After all, we
> only ever call .open at most once per connection.
I don't think so - backend_reopen will call plugin_open a second time.
As a test I added assert (conn->exportname == NULL) before this line
and it crashed in tests/test-retry.sh.
> >+++ b/server/protocol-handshake-newstyle.c
> >@@ -200,11 +200,29 @@ conn_recv_full (void *buf, size_t len, const char
*fmt, ...)
> >   * in that function, and must not cause any wire traffic.
> >   */
> >  static int
> >-finish_newstyle_options (uint64_t *exportsize)
> >+finish_newstyle_options (uint64_t *exportsize,
> >+                         const char *exportname_in, uint32_t
exportnamelen)
> >  {
> >    GET_CONN;
> >-  if (protocol_common_open (exportsize, &conn->eflags) == -1)
> >+  /* Since the exportname string passed here comes directly out of the
> >+   * NBD protocol make a temporary copy of the exportname into a
> >+   * \0-terminated buffer.
> >+   */
> >+  CLEANUP_FREE char *exportname = strndup (exportname_in,
exportnamelen);
> >+  if (exportname == NULL) {
> >+    nbdkit_error ("strndup: %m");
> >+    return -1;
> >+  }
> >+
> >+  if (conn->exportname_from_set_meta_context &&
> >+      strcmp (conn->exportname_from_set_meta_context, exportname)
!= 0) {
> >+    debug ("newstyle negotiation: NBD_OPT_SET_META_CONTEXT export
name \"%s\" ≠ final client exportname \"%s\", so discarding
the previous context",
> 
> Long line, and I don't know if we use UTF-8 in other debug messages
> or should stick to straight ascii.  Compilation may have a glitch if
> compiled under a different locale than the end binary runs in, but
> these days, it's uncommon to find someone running in a single-byte
> locale instead of UTF-8.
I thought we might be using ‘’ quotes (as we do in libguestfs) but I
see that we don't.  I think we should use Unicode characters more
where they are appropriate, but I'll break up this long line.  I also
added a comment.
> >@@ -148,7 +166,7 @@ ext2_prepare (struct nbdkit_next_ops *next_ops,
void *nxdata, void *handle,
> >    struct ext2_inode inode;
> >    int64_t r;
> >    CLEANUP_FREE char *name = NULL;
> >-  const char *fname = file ?: nbdkit_export_name ();
> >+  const char *fname = file ?: h->exportname;
> 
> Hmm - we already pass the same 'readonly' state to filter's
> .prepare, but not to backend_prepare(), which has to reconstruct it.
> Would it be easier to also change the signature of backend_prepare()
> to take both the original readonly and exportname passed to
> backend_open(), rather than making the filter have to save it off in
> the filter?  It looks like protocol-handshake.c is the only caller,
> and still has everything in scope at the time.
Possibly, although this would be a seperate change.

Thanks,

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 7/21/20 7:41 PM, Eric Blake wrote:
>> The handling of NBD_OPT_SET_META_CONTEXT interacted with the export
>> name (see commit 20b8509a9ccdab118ce7b7043be63bbad74f1e79).  I have
>> attempted to keep previous behaviour in this change, but note that
>> there is no regression test for this.  I added a debug message so we
>> can tell when this unusual case actually happens which should help
>> with diagnosis of problems.
> 
> Yeah, that commit specifically mentioned that I used gdb breakpoints in 
> qemu-io to test things, and was not interested in hacking libnbd at the 
> time.  Although now that we are debating about exposing nbd_opt_* 
> commands in libnbd to someone that opts in, maybe that _could_ be a case 
> to write such a regression test.  Meanwhile, I'll just try to fire up 
> another gdb session to prove to myself that things still work.
> 
Here's snippets of my gdb session proving it does indeed still work (now 
that you've actually pushed your patches):

$ ./nbdkit -U - -v memory 1 --run \
   'gdb --args qemu-img map --output=json "$uri"'
...
Reading symbols from qemu-img...
(gdb) b nbd_send_meta_query
Breakpoint 1 at 0xf3773: file /home/eblake/qemu/nbd/client.c, line 653.
(gdb) r
Starting program: /home/eblake/qemu/qemu-img map --output=json 
nbd+unix://\?socket=/tmp/nbdkityhz8Ab/socket
...
nbdkit: memory[1]: debug: newstyle negotiation: 
NBD_OPT_STRUCTURED_REPLY: client requested structured replies

Thread 1 "qemu-img" hit Breakpoint 1, nbd_send_meta_query 
(ioc=0x55555583f720,
     opt=10, export=0x5555557e7e80 "", query=0x55555571994c 
"base:allocation",
     errp=0x7fffffffcc20) at /home/eblake/qemu/nbd/client.c:653
653	    uint32_t export_len = strlen(export);
(gdb) p export="a"
$1 = 0x7fffecd80fa0 "a"
(gdb) c
...
nbdkit: memory[1]: debug: newstyle negotiation: 
NBD_OPT_SET_META_CONTEXT: client requested export 'a'
nbdkit: memory[1]: debug: newstyle negotiation: 
NBD_OPT_SET_META_CONTEXT: set count: 1
nbdkit: memory[1]: debug: newstyle negotiation: 
NBD_OPT_SET_META_CONTEXT: set base:allocation
nbdkit: memory[1]: debug: newstyle negotiation: 
NBD_OPT_SET_META_CONTEXT: replying with base:allocation id 1
nbdkit: memory[1]: debug: newstyle negotiation: 
NBD_OPT_SET_META_CONTEXT: reply complete
nbdkit: memory[1]: debug: newstyle negotiation: NBD_OPT_GO: client 
requested export ''
nbdkit: memory[1]: debug: newstyle negotiation: NBD_OPT_SET_META_CONTEXT 
export name "a" ≠ final client exportname "", so discarding
the previous
context
...

nbdkit: memory.1: error: invalid request: NBD_CMD_BLOCK_STATUS: 
base:allocation was not negotiated
nbdkit: debug: starting worker thread memory.13
nbdkit: debug: starting worker thread memory.14
nbdkit: debug: starting worker thread memory.15
nbdkit: memory.1: debug: sending error reply: Invalid argument
qemu-img: Could not read file metadata: Invalid argument
...

So using gdb to hack in a different name shows the nbdkit code works; it 
also shows that qemu does NOT expect NBD_CMD_BLOCK_STATUS to fail if 
NBD_OPT_SET_META_CONTEXT succeeded (but that's understandable - qemu 
always sends the same export name; the fault lies in me changing state 
with gdb behind qemu's back).

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