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

[Eric Blake]

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