Libguestfs - Jul 2020 - Re: [nbdkit PATCH] server: Reinstate limited use of -e/-exportname.

While we are unlikely to change our decision that -e should not
control our response to NBD_OPT_LIST (because we intend to add a new
callback .extents_list for that), it turns out that it is a lot easier
to write:

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

than it is to write the equivalent:

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

and our test-long-name.sh was evidence of that fact.

Let's revert that part of the patch, but improve the documentation to
mention that -e is now useful _only_ with --run, and only if the
client uses it.

This partially reverts commit e71178e9b71403, although for -Wshadow
reasons, we now have to name the global variable export_name due to
other code in the meantime using parameters named exportname.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-captive.pod  | 12 +++++++++++-
 docs/nbdkit-protocol.pod |  2 +-
 docs/nbdkit.pod          | 18 ++++++++++++++++++
 docs/synopsis.txt        |  3 ++-
 server/internal.h        |  1 +
 server/captive.c         | 20 +++++++++++++++++---
 server/main.c            |  3 ++-
 tests/test-long-name.sh  | 10 +++++-----
 8 files changed, 57 insertions(+), 12 deletions(-)

diff --git a/docs/nbdkit-captive.pod b/docs/nbdkit-captive.pod
index f1ac9fc4..94e79c59 100644
--- a/docs/nbdkit-captive.pod
+++ b/docs/nbdkit-captive.pod
@@ -53,7 +53,9 @@ The following shell variables are available in the
I<--run> argument:
 A URI that refers to the nbdkit port or socket in the preferred form
 documented by the NBD project.  As this variable may contain a bare
 C<?> for Unix sockets, it is safest to use $uri within double quotes
-to avoid unintentional globbing.
+to avoid unintentional globbing.  For plugins that support distinct
+data based on export names, the B<-e> option to nbdkit controls which
+export name will be set in the URI.

 =item C<$nbd>

@@ -72,6 +74,14 @@ 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 export name (which may be "") that the process should use when
+connecting to nbdkit, as set by the B<-e> (B<--exportname>) command
+line option of nbdkit.  This only matters to plugins that
+differentiate what they serve based on the export name requested by
+the client.
+
 =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 e0e4042b..8fe8c67e 100644
--- a/docs/nbdkit-protocol.pod
+++ b/docs/nbdkit-protocol.pod
@@ -102,7 +102,7 @@ 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.

 Versions of nbdkit before 1.16 could advertise a single export name to
-clients, specified through the now deprecated I<-e> option.  In nbdkit
+clients, via a now deprecated side effect of the 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.

diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index b204709b..e23cc90f 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -214,6 +214,24 @@ 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 a preferred exportname to expose in the shell environment created
+during B<--run>.  The use of this option without B<--run> has no
+effect.  This option does I<not> change what nbdkit advertises as a
+server, but can aid in writing a captive client that wants to access
+particular content from a plugin that differentiates content based on
+the client's choice of export name.
+
+If not set, the B<--run> environment is set to access the default
+exportname C<""> (empty string).
+
 =item B<-f>

 =item B<--foreground>
diff --git a/docs/synopsis.txt b/docs/synopsis.txt
index caf49922..07b9dcff 100644
--- a/docs/synopsis.txt
+++ b/docs/synopsis.txt
@@ -1,4 +1,5 @@
-nbdkit [-D|--debug PLUGIN|FILTER|nbdkit.FLAG=N] [--exit-with-parent]
+nbdkit [-D|--debug PLUGIN|FILTER|nbdkit.FLAG=N]
+       [-e|--exportname EXPORTNAME] [--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 4911a49b..1acbbb06 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -108,6 +108,7 @@ enum log_to {
 };

 extern struct debug_flag *debug_flags;
+extern const char *export_name;
 extern bool foreground;
 extern const char *ipaddr;
 extern enum log_to log_to;
diff --git a/server/captive.c b/server/captive.c
index 53b82782..a8947d7c 100644
--- a/server/captive.c
+++ b/server/captive.c
@@ -61,6 +61,9 @@ run_command (void)
   if (!run)
     return;

+  if (!export_name)
+    export_name = "";
+
   fp = open_memstream (&cmd, &len);
   if (fp == NULL) {
     perror ("open_memstream");
@@ -72,15 +75,26 @@ run_command (void)
   if (port) {
     fprintf (fp, "nbd://localhost:");
     shell_quote (port, fp);
+    if (strcmp (export_name, "") != 0) {
+      putc ('/', fp);
+      uri_quote (export_name, fp);
+    }
   }
   else if (unixsocket) {
-    fprintf (fp, "nbd+unix://\\?socket=");
+    fprintf (fp, "nbd+unix://");
+    if (strcmp (export_name, "") != 0) {
+      putc ('/', fp);
+      uri_quote (export_name, fp);
+    }
+    fprintf (fp, "\\?socket=");
     uri_quote (unixsocket, fp);
   }
   putc ('\n', fp);

-  /* $exportname is for backwards compatibility. */
-  fprintf (fp, "exportname=\n");
+  /* Expose $exportname. */
+  fprintf (fp, "exportname=");
+  shell_quote (export_name, 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 d815fe12..2f0aaf07 100644
--- a/server/main.c
+++ b/server/main.c
@@ -79,6 +79,7 @@ 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 *export_name;        /* -e */
 bool foreground;                /* -f */
 const char *ipaddr;             /* -i */
 enum log_to log_to = LOG_TO_DEFAULT; /* --log */
@@ -344,7 +345,7 @@ main (int argc, char *argv[])
       break;

     case 'e':
-      /* Does nothing, ignored for compatibility with older nbdkit. */
+      export_name = optarg;
       break;

     case 'f':
diff --git a/tests/test-long-name.sh b/tests/test-long-name.sh
index 1da2515a..897f8e94 100755
--- a/tests/test-long-name.sh
+++ b/tests/test-long-name.sh
@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 # nbdkit
-# Copyright (C) 2019 Red Hat Inc.
+# 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
@@ -53,10 +53,6 @@ name1k=$name256$name256$name256$name256
 name4k=$name1k$name1k$name1k$name1k
 almost4k=${name4k%8$name16}

-# Test behavior of -e: accept 4k max, reject longer
-nbdkit -U - -e $name4k null --run true || fail=1
-nbdkit -U - -e a$name4k null --run true && fail=1
-
 # Test that $exportname and $uri reflect the name
 out=$(nbdkit -U - -e $name4k null --run 'echo $exportname')
 if test "$name4k" != "$out"; then
@@ -110,6 +106,9 @@ assert h.get_size() == 0
 EOF
 '

+# TODO when we add .list_extents, we should test a plugin advertising
+# a name near 4k in length. nbdkit -e no longer does this
+if false; then
 # The rest of this test uses the ‘qemu-nbd --list’ option added in qemu 4.0.
 if ! qemu-nbd --help | grep -sq -- --list; then
     echo "$0: skipping because qemu-nbd does not support the --list
option"
@@ -118,5 +117,6 @@ fi

 # Test response to NBD_OPT_LIST
 nbdkit -U - -e $name4k null --run 'qemu-nbd --list -k $unixsocket' ||
fail=1
+fi

 exit $fail
-- 
2.27.0

On Wed, Jul 22, 2020 at 12:58:07PM -0500, Eric Blake wrote:
[...]

This is fine, but there is a minor thing:
>  docs/nbdkit-protocol.pod |  2 +-
In the original patch I removed this hunk from 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] [...]
+        [...]
 
I guess we should add that back.

docs/nbdkit-captive.pod also has a synopsis, but it didn't cover -e
before (probably because I didn't realise that -e affected --run).  So
maybe add something to that file as well?

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 7/23/20 4:45 PM, Richard W.M. Jones wrote:
> On Wed, Jul 22, 2020 at 12:58:07PM -0500, Eric Blake wrote:
> [...]
> 
> This is fine, but there is a minor thing:
> 
>>   docs/nbdkit-protocol.pod |  2 +-
> 
> In the original patch I removed this hunk from 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] [...]
> +        [...]
>   
> I guess we should add that back.
Probably not here, because -e no longer affects the protocol,
> 
> docs/nbdkit-captive.pod also has a synopsis, but it didn't cover -e
> before (probably because I didn't realise that -e affected --run).  So
> maybe add something to that file as well?
but yes, adding mention of -e here makes sense.  I'll make that change, 
then push.

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