Libguestfs - Jul 2020 - Re: [PATCH nbdkit] server: Deprecate the -e/--exportname parameter.

This parameter provided a name for the "default export" -- ie. the one
and only export returned to the client by NBD_OPT_LIST.  But nbdkit
traditionally didn't care what export name the client requested.
Since 1.16 plugins have been able to serve different content per
export name (and return errors for unknown exports), but the -e option
didn't reflect that and only created confusion.

What we actually have to do is ask the plugin what exports it provides
and return that list to the client.  This is for future work.

This commit deprecates and hides the parameter.  If used the option
now does nothing at all.

There are some visible but very minor changes resulting from this
commit:

(1) NBD_OPT_LIST advertises a single export called "" (instead of the
    -e parameter).  We intend to replace this implementation soon with a
    correct one.

(2) The --run option no longer sets $exportname (to -e) nor puts the
    export name into the $uri.  However this was always the wrong
    thing to do since export names are per connection, not per server.
    Existing --run scripts will see $exportname expand to "" which is
    most likely what they saw before and overwhelmingly more likely to
    be correct than if the -e option had been used.

(3) The -e option had the side-effect of forcing the newstyle
    protocol, so weirdly this worked and made the server use newstyle:

      nbdkit -o -e '' ...

    but this would fail with an error at start-up:

      nbdkit -e '' -o ...

    I thought it best to remove this odd behaviour completely.

(4) I have temporarily disabled tests/test-long-name.sh.  This is
    still a valid test, but we will have to rewrite it to use
    (probably) sh or eval plugins once we have an implementation of
    list_exports.
---
 docs/nbdkit-captive.pod              | 12 ++----------
 docs/nbdkit-protocol.pod             | 15 +++++++--------
 docs/nbdkit.pod                      | 13 -------------
 docs/synopsis.txt                    |  3 +--
 server/internal.h                    |  1 -
 server/captive.c                     | 18 +-----------------
 server/main.c                        | 21 +--------------------
 server/protocol-handshake-newstyle.c | 12 +++++++++---
 tests/test-data-7E.sh                |  2 +-
 tests/test-data-raw.sh               |  3 +--
 tests/test-long-name.sh              |  4 ++++
 11 files changed, 27 insertions(+), 77 deletions(-)

diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod
index 09628367..66c5c81c 100644
--- a/docs/nbdkit-captive.pod
+++ b/docs/nbdkit-captive.pod
@@ -47,14 +47,12 @@ The following shell variables are available in the
I<--run> argument:
 =item C<$uri>
 
 A URI that refers to the nbdkit port or socket in the preferred form
-documented by the NBD project.  If nbdkit was started with the B<-e>
-option to set the preferred export name, this is included in the URI.
+documented by the NBD project.
 
 =item C<$nbd>
 
 An older URL that refers to the nbdkit port or socket in a manner
-specific to certain tools.  This form does not include an export name,
-even if B<-e> was used.
+specific to certain tools.
 
 Note there is some magic here, since qemu and guestfish URLs have a
 different format, so nbdkit tries to guess which you are running.  If
@@ -68,12 +66,6 @@ If E<ne> "", the port number that nbdkit is
listening on.
 
 If E<ne> "", the Unix domain socket that nbdkit is listening
on.
 
-=item C<$exportname>
-
-The default export name (which may be "") that nbdkit will advertise
-in response to NBD_OPT_LIST.  This comes from the B<-e>
-(B<--exportname>) command line option.
-
 =back
 
 I<--run> implies I<--foreground>.  It is not possible, and probably
diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod
index d90993fb..172d1326 100644
--- a/docs/nbdkit-protocol.pod
+++ b/docs/nbdkit-protocol.pod
@@ -5,7 +5,7 @@ nbdkit - which parts of the NBD protocol nbdkit supports
 =head1 SYNOPSIS
 
  nbdkit [-n|--newstyle] [--mask-handshake MASK] [--no-sr] [-o|--oldstyle]
-        [-e|--exportname EXPORTNAME] [...]
+        [...]
 
 =head1 DESCRIPTION
 
@@ -29,9 +29,6 @@ can support (in some cases, this can be limited by what
callbacks the
 plugin handles), even if the client does not negotiate to use all
 advertised features.
 
-Use the I<-e> or I<--exportname> flag to set the optional
exportname
-for the newstyle protocol.
-
 Nbdkit also includes some options that are useful mainly when
 performing integration tests, for proving whether clients have sane
 fallback behavior when dealing various older servers permitted by the
@@ -101,11 +98,13 @@ Supported in nbdkit E<ge> 1.1.12, and the default in
nbdkit E<ge> 1.3.
 
 =item export names
 
-Supported in nbdkit E<ge> 1.1.12.
+Partially supported in nbdkit E<ge> 1.1.12.  Support for plugins to
+read the client export name added in nbdkit E<ge> 1.15.2.
 
-nbdkit can advertise an export name, but ignores any export name sent
-by the client.  nbdkit does I<not> support serving different data on
-different export names.
+Versions of nbdkit before 1.16 could advertize a single export name to
+clients, specified through the now deprecated I<-e> option.  In nbdkit
+1.15.2, plugins could read the client requested export name using
+C<nbdkit_export_name()> and serve different content.
 
 =item C<NBD_FLAG_NO_ZEROES>
 
diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index a6cf988c..b204709b 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -214,19 +214,6 @@ An alternative to this is L<nbdkit-captive(1)/CAPTIVE
NBDKIT>.
 
 This option implies I<--foreground>.
 
-=item B<-e> EXPORTNAME
-
-=item B<--export> EXPORTNAME
-
-=item B<--export-name> EXPORTNAME
-
-=item B<--exportname> EXPORTNAME
-
-Set the exportname.
-
-If not set, exportname C<""> (empty string) is used. 
Exportnames are
-not allowed with the oldstyle protocol.
-
 =item B<-f>
 
 =item B<--foreground>
diff --git a/docs/synopsis.txt b/docs/synopsis.txt
index 07b9dcff..caf49922 100644
--- a/docs/synopsis.txt
+++ b/docs/synopsis.txt
@@ -1,5 +1,4 @@
-nbdkit [-D|--debug PLUGIN|FILTER|nbdkit.FLAG=N]
-       [-e|--exportname EXPORTNAME] [--exit-with-parent]
+nbdkit [-D|--debug PLUGIN|FILTER|nbdkit.FLAG=N] [--exit-with-parent]
        [--filter FILTER ...] [-f|--foreground]
        [-g|--group GROUP] [-i|--ipaddr IPADDR]
        [--log stderr|syslog|null]
diff --git a/server/internal.h b/server/internal.h
index 68c53366..ebc63a52 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -108,7 +108,6 @@ enum log_to {
 };
 
 extern struct debug_flag *debug_flags;
-extern const char *exportname;
 extern bool foreground;
 extern const char *ipaddr;
 extern enum log_to log_to;
diff --git a/server/captive.c b/server/captive.c
index f8107604..aa717870 100644
--- a/server/captive.c
+++ b/server/captive.c
@@ -61,8 +61,6 @@ run_command (void)
   if (!run)
     return;
 
-  assert (exportname != NULL);
-
   fp = open_memstream (&cmd, &len);
   if (fp == NULL) {
     perror ("open_memstream");
@@ -74,27 +72,13 @@ run_command (void)
   if (port) {
     fprintf (fp, "nbd://localhost:");
     shell_quote (port, fp);
-    if (strcmp (exportname, "") != 0) {
-      putc ('/', fp);
-      uri_quote (exportname, fp);
-    }
   }
   else if (unixsocket) {
-    fprintf (fp, "nbd+unix://");
-    if (strcmp (exportname, "") != 0) {
-      putc ('/', fp);
-      uri_quote (exportname, fp);
-    }
-    fprintf (fp, "\\?socket=");
+    fprintf (fp, "nbd+unix://\\?socket=");
     uri_quote (unixsocket, fp);
   }
   putc ('\n', fp);
 
-  /* Expose $exportname. */
-  fprintf (fp, "exportname=");
-  shell_quote (exportname, fp);
-  putc ('\n', fp);
-
   /* Construct older $nbd "URL".  Unfortunately guestfish and qemu
take
    * different syntax, so try to guess which one we need.
    */
diff --git a/server/main.c b/server/main.c
index c432f5bd..d815fe12 100644
--- a/server/main.c
+++ b/server/main.c
@@ -79,7 +79,6 @@ static bool is_config_key (const char *key, size_t len);
 
 struct debug_flag *debug_flags; /* -D */
 bool exit_with_parent;          /* --exit-with-parent */
-const char *exportname;         /* -e */
 bool foreground;                /* -f */
 const char *ipaddr;             /* -i */
 enum log_to log_to = LOG_TO_DEFAULT; /* --log */
@@ -345,13 +344,7 @@ main (int argc, char *argv[])
       break;
 
     case 'e':
-      exportname = optarg;
-      if (strnlen (exportname, NBD_MAX_STRING + 1) > NBD_MAX_STRING) {
-        nbdkit_error ("export name too long");
-        exit (EXIT_FAILURE);
-      }
-      /* TODO: Check that name is valid UTF-8? */
-      newstyle = true;
+      /* Does nothing, ignored for compatibility with older nbdkit. */
       break;
 
     case 'f':
@@ -487,18 +480,6 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }
 
-  /* Oldstyle protocol + exportname not allowed. */
-  if (!newstyle && exportname != NULL) {
-    fprintf (stderr,
-             "%s: cannot use oldstyle protocol (-o) and exportname
(-e)\n",
-             program_name);
-    exit (EXIT_FAILURE);
-  }
-
-  /* If exportname was not set on the command line, use "". */
-  if (exportname == NULL)
-    exportname = "";
-
   /* --tls=require and oldstyle won't work. */
   if (tls == 2 && !newstyle) {
     fprintf (stderr,
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 192d00f3..fa44f253 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -75,12 +75,15 @@ send_newstyle_option_reply (uint32_t option, uint32_t reply)
   return 0;
 }
 
+/* Reply to NBD_OPT_LIST with a single empty export name.
+ * TODO: Ask the plugin for the list of exports.
+ */
 static int
 send_newstyle_option_reply_exportname (uint32_t option, uint32_t reply)
 {
   GET_CONN;
   struct nbd_fixed_new_option_reply fixed_new_option_reply;
-  size_t name_len = strlen (exportname);
+  const size_t name_len = 0;    /* length of export name */
   uint32_t len;
 
   fixed_new_option_reply.magic = htobe64 (NBD_REP_MAGIC);
@@ -100,11 +103,14 @@ send_newstyle_option_reply_exportname (uint32_t option,
uint32_t reply)
                   name_of_nbd_opt (option), "sending length");
     return -1;
   }
+#if 0
+  /* If we were sending a non-"" export name, this is what we'd
use. */
   if (conn->send (exportname, name_len, 0) == -1) {
     nbdkit_error ("write: %s: %s: %m",
                   name_of_nbd_opt (option), "sending export name");
     return -1;
   }
+#endif
 
   return 0;
 }
@@ -364,8 +370,8 @@ negotiate_handshake_newstyle_options (void)
       }
 
       /* Send back the exportname. */
-      debug ("newstyle negotiation: %s: advertising export
'%s'",
-             name_of_nbd_opt (option), exportname);
+      debug ("newstyle negotiation: %s: advertising export
\"\"",
+             name_of_nbd_opt (option));
       if (send_newstyle_option_reply_exportname (option, NBD_REP_SERVER) == -1)
         return -1;
 
diff --git a/tests/test-data-7E.sh b/tests/test-data-7E.sh
index e06d3c7e..c4fa86b1 100755
--- a/tests/test-data-7E.sh
+++ b/tests/test-data-7E.sh
@@ -45,7 +45,7 @@ rm -f $files
 cleanup_fn rm -f $files
 
 # Run nbdkit.
-start_nbdkit -P data-7E.pid -U $sock --export= \
+start_nbdkit -P data-7E.pid -U $sock \
        --filter=partition \
        data size=7E partition=1 \
        data="
diff --git a/tests/test-data-raw.sh b/tests/test-data-raw.sh
index 36ee8028..446e2093 100755
--- a/tests/test-data-raw.sh
+++ b/tests/test-data-raw.sh
@@ -44,8 +44,7 @@ rm -f $files
 cleanup_fn rm -f $files
 
 # Run nbdkit.
-start_nbdkit -P data-raw.pid -U $sock --export "" \
-       data raw=123 size=512
+start_nbdkit -P data-raw.pid -U $sock data raw=123 size=512
 
 nbdsh --connect "nbd+unix://?socket=$sock" \
       -c '
diff --git a/tests/test-long-name.sh b/tests/test-long-name.sh
index 6a512603..f70f492d 100755
--- a/tests/test-long-name.sh
+++ b/tests/test-long-name.sh
@@ -38,6 +38,10 @@ fail=0
 
 # Test handling of NBD maximum string length of 4k.
 
+echo "$0: This test needs to be rewritten to use alternate method to"
+echo "send back the export name to the client."
+exit 77
+
 requires qemu-io --version
 requires qemu-nbd --version
 requires nbdsh -c 'exit(not h.supports_uri())'
-- 
2.27.0

On 7/21/20 10:10 AM, Richard W.M. Jones wrote:
> This parameter provided a name for the "default export" -- ie.
the one
> and only export returned to the client by NBD_OPT_LIST.  But nbdkit
> traditionally didn't care what export name the client requested.
> Since 1.16 plugins have been able to serve different content per
> export name (and return errors for unknown exports), but the -e option
> didn't reflect that and only created confusion.
> 
> What we actually have to do is ask the plugin what exports it provides
> and return that list to the client.  This is for future work.
> 
> This commit deprecates and hides the parameter.  If used the option
> now does nothing at all.
> 
> There are some visible but very minor changes resulting from this
> commit:
> 
> (1) NBD_OPT_LIST advertises a single export called "" (instead of
the
>      -e parameter).  We intend to replace this implementation soon with a
>      correct one.
And in the meantime, we still allow clients to connect with any name at 
all, and that still makes no difference for plugins that don't read the 
client's request.
> 
> (2) The --run option no longer sets $exportname (to -e) nor puts the
>      export name into the $uri.  However this was always the wrong
>      thing to do since export names are per connection, not per server.
>      Existing --run scripts will see $exportname expand to ""
which is
>      most likely what they saw before and overwhelmingly more likely to
>      be correct than if the -e option had been used.
> 
> (3) The -e option had the side-effect of forcing the newstyle
>      protocol, so weirdly this worked and made the server use newstyle:
> 
>        nbdkit -o -e '' ...
> 
>      but this would fail with an error at start-up:
> 
>        nbdkit -e '' -o ...
> 
>      I thought it best to remove this odd behaviour completely.
If we wanted, we could mandate that if -o is in effect, any use of -e 
must be '' (that's what I did in qemu-nbd prior to when I ripped 
old-style protocol out completely; and it is similar to what 'qemu-nbd 
--list' does - basically, the code is most consistent when it pretends 
that the export name for any old-style connection is "", rather than 
forbidden).  But I'm also fine with just dropping the odd behavior (-e 
is now a no-op, whether you use new- or old-style).
> 
> (4) I have temporarily disabled tests/test-long-name.sh.  This is
>      still a valid test, but we will have to rewrite it to use
>      (probably) sh or eval plugins once we have an implementation of
>      list_exports.
> ---
> --- a/docs/nbdkit-captive.pod
> -nbdkit can advertise an export name, but ignores any export name sent
> -by the client.  nbdkit does I<not> support serving different data on
> -different export names.
> +Versions of nbdkit before 1.16 could advertize a single export name to
advertise (particularly since that was the spelling in the previous text).
> +++ b/server/captive.c
> @@ -61,8 +61,6 @@ run_command (void)
>     if (!run)
>       return;
>   
> -  assert (exportname != NULL);
> -
>     fp = open_memstream (&cmd, &len);
>     if (fp == NULL) {
>       perror ("open_memstream");
> @@ -74,27 +72,13 @@ run_command (void)
>     if (port) {
>       fprintf (fp, "nbd://localhost:");
>       shell_quote (port, fp);
> -    if (strcmp (exportname, "") != 0) {
> -      putc ('/', fp);
> -      uri_quote (exportname, fp);
> -    }
>     }
>     else if (unixsocket) {
> -    fprintf (fp, "nbd+unix://");
> -    if (strcmp (exportname, "") != 0) {
> -      putc ('/', fp);
> -      uri_quote (exportname, fp);
> -    }
> -    fprintf (fp, "\\?socket=");
> +    fprintf (fp, "nbd+unix://\\?socket=");
>       uri_quote (unixsocket, fp);
>     }
Hmm.  When we do allow plugins to advertise exportnames, we'll probably 
want a way to specify the right exportname (suppose a plugin advertises 
both "a" and "b" and serves different content accordingly,
--run then
has to know which one to go with, rather than assuming "" will work). 
But I don't mind that being for later.
>     putc ('\n', fp);
>   
> -  /* Expose $exportname. */
> -  fprintf (fp, "exportname=");
> -  shell_quote (exportname, fp);
> -  putc ('\n', fp);
> -
This breaks anyone that does 'set -u' in a shell script, or where --run 
triggers a binary that then calls getenv("exportname") and expects a 
non-NULL value.  I'd still leave 'exportname=\n' in the --run
command
line, to guarantee it is set but empty.

Otherwise makes sense to me.

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

I realise I forgot the update the filter documentation, so the patch
below has been squashed into my copy.  The filter documentation
probably needs a review to make sure it is up to date as certain
functions like reopen are missing.

Rich.


diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod
index acac3e50..f0ff9ef2 100644
--- a/docs/nbdkit-filter.pod
+++ b/docs/nbdkit-filter.pod
@@ -329,7 +329,7 @@ an error message and return C<-1>.
 =head2 C<.open>
 
  void * (*open) (nbdkit_next_open *next, void *nxdata,
-                 int readonly);
+                 int readonly, const char *exportname);
 
 This is called when a new client connection is opened and can be used
 to allocate any per-connection data structures needed by the filter.
@@ -345,17 +345,22 @@ If there is an error, C<.open> should call
C<nbdkit_error> with an
 error message and return C<NULL>.
 
 This callback is optional, but if provided, it must call C<next>,
-passing a value for C<readonly> according to how the filter plans to
-use the plugin.  Typically, the filter passes the same value as it
-received, or passes true to provide a writable layer on top of a
-read-only backend.  However, it is also acceptable to attempt write
-access to the plugin even if this filter is readonly, such as when a
-file system mounted read-only still requires write access to the
-underlying device in case a journal needs to be replayed for
-consistency as part of the mounting process.  The filter should
-generally call C<next> as its first step, to allocate from the plugin
-outwards, so that C<.close> running from the outer filter to the
-plugin will be in reverse.
+passing C<readonly> and C<exportname> possibly modified according
to
+how the filter plans to use the plugin.  Typically, the filter passes
+the same values as it received, or passes readonly=true to provide a
+writable layer on top of a read-only backend.  However, it is also
+acceptable to attempt write access to the plugin even if this filter
+is readonly, such as when a file system mounted read-only still
+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 filter should generally call C<next> as its first step, to
+allocate from the plugin outwards, so that C<.close> running from the
+outer filter to the plugin will be in reverse.
 
 =head2 C<.close>
 


-- 
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 12:46 PM, Eric Blake wrote:
>> (2) The --run option no longer sets $exportname (to -e) nor puts the
>>      export name into the $uri.  However this was always the wrong
>>      thing to do since export names are per connection, not per server.
>>      Existing --run scripts will see $exportname expand to ""
which is
>>      most likely what they saw before and overwhelmingly more likely to
>>      be correct than if the -e option had been used.
>>
The more I think about it, the more I disagree with disabling this.  Or, 
put another way, I think that the _only_ time -e makes sense is _when_ 
you are using --run.  Consider:

nbdkit -U - -e foo info --run 'nbdsh -u $uri -c "print(h.pread(3,
0))"'
nbdkit -U - -e bar info --run 'nbdsh -u $uri -c "print(h.pread(3,
0))"'

Of course, you can accomplish the same with:

nbdkit -U - info --run 'nbdsh -u nbd+unix:///foo\?socket=$unixsocket \
   -c "print(h.pread(3, 0))"'

but that's a lot more painful to write.

We _don't_ need to advertise 'foo' or 'bar' over
NBD_OPT_INFO (at least,
not for plugins that don't implement the forthcoming .export_list 
callback), but _do_ need a way for the captive application (nbdsh in 
this case) to know _which_ export the captive should connect to.

And, if we reinstate _just_ that usage of -e,...
>> (4) I have temporarily disabled tests/test-long-name.sh.  This is
>>      still a valid test, but we will have to rewrite it to use
>>      (probably) sh or eval plugins once we have an implementation of
>>      list_exports.
>> ---
...we may not have to disable this test after all.

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