Libguestfs - Sep 2019 - Re: [PATCH nbdkit v2 3/3] tests: Add a simple test of nbdkit_export_name.

The previous incomplete patch was here:
https://www.redhat.com/archives/libguestfs/2019-September/msg00049.html
based on earlier discussion here:
https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html

In v2:

 - The previous patch was incomplete.  This version completes it by
   adding tests and extending nbdkit-sh-plugin.

 - nbdkit_export_name now returns NULL for error, setting
   nbdkit_error.  The only possible error currently is if we're not
   called from a connected callback.

 - For oldstyle protocol, "" is returned.

 - Add some clarifying text to docs for can_multi_conn.

 - Use strcpy in one place instead of memcpy (but leave the other one
   alone).

As before this patch doesn't attempt to resolve any problems with our
current handling of -e, NBD_OPT_LIST, etc.

Rich.

This allows plugins (or filters) to read the export name which was
passed to the server from the client.
---
 TODO                                 |  8 +++++++
 docs/nbdkit-plugin.pod               | 29 ++++++++++++++++++++++
 include/nbdkit-common.h              |  1 +
 server/connections.c                 | 36 ++++++++++++++++++----------
 server/internal.h                    |  1 +
 server/nbdkit.syms                   |  1 +
 server/protocol-handshake-newstyle.c | 31 ++++++++++++++----------
 server/public.c                      | 13 ++++++++++
 8 files changed, 95 insertions(+), 25 deletions(-)

diff --git a/TODO b/TODO
index 49b60b1..2468d74 100644
--- a/TODO
+++ b/TODO
@@ -62,6 +62,12 @@ General ideas for improvements
   and also look at the implementation of the -swap option in
   nbd-client.
 
+* Clients should be able to list export names supported by plugins.
+  Current behaviour is not really correct: We only list the -e
+  parameter from the command line, which is different from the export
+  name(s) that a plugin might want to support.  Probably we should
+  deprecate the -e option entirely since it does nothing useful.
+
 Suggestions for plugins
 -----------------------
 
@@ -190,3 +196,5 @@ using ‘#define NBDKIT_API_VERSION <version>’.
   value) strings.  nbdkit should know the possible keys for the plugin
   and filters, and the type of the values, and both check and parse
   them for the plugin.
+
+* Modify open() API so it takes an export name parameter.
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index bb162e4..219906e 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -362,6 +362,32 @@ requested, or -1 after calling C<nbdkit_error> if
there is no point in
 continuing the current command.  Attempts to sleep more than
 C<INT_MAX> seconds are treated as an error.
 
+=head1 EXPORT NAME
+
+If the client negotiated an NBD export name with nbdkit then plugins
+may read this from any connected callbacks.  Nbdkit's normal behaviour
+is to accept any export name passed by the client, log it in debug
+output, but otherwise ignore it.  By using C<nbdkit_export_name>
+plugins may choose to filter by export name or serve different
+content.
+
+=head2 C<nbdkit_export_name>
+
+ const char *nbdkit_export_name (void);
+
+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.
+
+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<'/'>
+character.  The NBD protocol describes the empty string (C<"">)
as a
+representing a "default export" or to be used in cases where the
+export name does not make sense.
+
+On error, nbdkit_error is called and the call returns C<NULL>.
+
 =head1 CALLBACKS
 
 =head2 C<.name>
@@ -709,6 +735,9 @@ request or setting C<NBDKIT_FLAG_FUA> on a request
must be visible
 across all connections to the plugin before the plugin replies to that
 request.
 
+Properly working clients should send the same export name for each of
+these connections.
+
 If you use Linux L<nbd-client(8)> option S<I<-C num>> with
 S<num E<gt> 1> then Linux checks this flag and will refuse to
connect
 if C<.can_multi_conn> is false.
diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h
index 42d94a1..acf0abd 100644
--- a/include/nbdkit-common.h
+++ b/include/nbdkit-common.h
@@ -86,6 +86,7 @@ extern int nbdkit_parse_bool (const char *str);
 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);
 
 struct nbdkit_extents;
 extern int nbdkit_add_extent (struct nbdkit_extents *,
diff --git a/server/connections.c b/server/connections.c
index 8ef5e57..819f7b8 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -255,11 +255,18 @@ new_connection (int sockin, int sockout, int nworkers)
     perror ("malloc");
     return NULL;
   }
+
+  conn->status_pipe[0] = conn->status_pipe[1] = -1;
+
+  conn->exportname = strdup ("");
+  if (conn->exportname == NULL) {
+    perror ("strdup");
+    goto error;
+  }
   conn->handles = calloc (backend->i + 1, sizeof *conn->handles);
   if (conn->handles == NULL) {
     perror ("malloc");
-    free (conn);
-    return NULL;
+    goto error;
   }
   conn->nr_handles = backend->i + 1;
   memset (conn->handles, -1, conn->nr_handles * sizeof
*conn->handles);
@@ -272,8 +279,7 @@ new_connection (int sockin, int sockout, int nworkers)
 #ifdef HAVE_PIPE2
     if (pipe2 (conn->status_pipe, O_NONBLOCK | O_CLOEXEC)) {
       perror ("pipe2");
-      free (conn);
-      return NULL;
+      goto error;
     }
 #else
     /* If we were fully parallel, then this function could be
@@ -289,29 +295,24 @@ new_connection (int sockin, int sockout, int nworkers)
     lock_request (NULL);
     if (pipe (conn->status_pipe)) {
       perror ("pipe");
-      free (conn);
       unlock_request (NULL);
-      return NULL;
+      goto error;
     }
     if (set_nonblock (set_cloexec (conn->status_pipe[0])) == -1) {
       perror ("fcntl");
       close (conn->status_pipe[1]);
-      free (conn);
       unlock_request (NULL);
-      return NULL;
+      goto error;
     }
     if (set_nonblock (set_cloexec (conn->status_pipe[1])) == -1) {
       perror ("fcntl");
       close (conn->status_pipe[0]);
-      free (conn);
       unlock_request (NULL);
-      return NULL;
+      goto error;
     }
     unlock_request (NULL);
 #endif
   }
-  else
-    conn->status_pipe[0] = conn->status_pipe[1] = -1;
   conn->sockin = sockin;
   conn->sockout = sockout;
   pthread_mutex_init (&conn->request_lock, NULL);
@@ -329,6 +330,16 @@ new_connection (int sockin, int sockout, int nworkers)
   threadlocal_set_conn (conn);
 
   return conn;
+
+ error:
+  if (conn->status_pipe[0] >= 0)
+    close (conn->status_pipe[0]);
+  if (conn->status_pipe[1] >= 0)
+    close (conn->status_pipe[1]);
+  free (conn->exportname);
+  free (conn->handles);
+  free (conn);
+  return NULL;
 }
 
 static void
@@ -373,6 +384,7 @@ free_connection (struct connection *conn)
   pthread_mutex_destroy (&conn->status_lock);
 
   free (conn->handles);
+  free (conn->exportname);
   free (conn);
 }
 
diff --git a/server/internal.h b/server/internal.h
index ac5b894..1f72b01 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -181,6 +181,7 @@ struct connection {
   struct b_conn_handle *handles;
   size_t nr_handles;
 
+  char *exportname;
   uint32_t cflags;
   uint16_t eflags;
   bool using_tls;
diff --git a/server/nbdkit.syms b/server/nbdkit.syms
index 2a024ed..1fb1315 100644
--- a/server/nbdkit.syms
+++ b/server/nbdkit.syms
@@ -42,6 +42,7 @@
     nbdkit_add_extent;
     nbdkit_debug;
     nbdkit_error;
+    nbdkit_export_name;
     nbdkit_extents_count;
     nbdkit_extents_free;
     nbdkit_extents_new;
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 75465b7..87e0bcd 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -272,11 +272,17 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
       if (conn_recv_full (conn, data, optlen,
                           "read: %s: %m", name_of_nbd_opt (option))
== -1)
         return -1;
-      /* Apart from printing it, ignore the export name. */
+      /* Print the export name and save it in the connection. */
       data[optlen] = '\0';
-      debug ("newstyle negotiation: %s: "
-             "client requested export '%s' (ignored)",
+      debug ("newstyle negotiation: %s: client requested export
'%s'",
              name_of_nbd_opt (option), data);
+      free (conn->exportname);
+      conn->exportname = malloc (optlen+1);
+      if (conn->exportname == NULL) {
+        nbdkit_error ("malloc: %m");
+        return -1;
+      }
+      strcpy (conn->exportname, data);
 
       /* We have to finish the handshake by sending handshake_finish. */
       if (finish_newstyle_options (conn, &exportsize) == -1)
@@ -388,7 +394,6 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
         uint16_t nrinfos;
         uint16_t info;
         size_t i;
-        CLEANUP_FREE char *requested_exportname = NULL;
 
         /* Validate the name length and number of INFO requests. */
         memcpy (&exportnamelen, &data[0], 4);
@@ -411,19 +416,19 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
           continue;
         }
 
-        /* As with NBD_OPT_EXPORT_NAME we print the export name and then
-         * ignore it.
+        /* As with NBD_OPT_EXPORT_NAME we print the export name and
+         * save it in the connection.
          */
-        requested_exportname = malloc (exportnamelen+1);
-        if (requested_exportname == NULL) {
+        free (conn->exportname);
+        conn->exportname = malloc (exportnamelen+1);
+        if (conn->exportname == NULL) {
           nbdkit_error ("malloc: %m");
           return -1;
         }
-        memcpy (requested_exportname, &data[4], exportnamelen);
-        requested_exportname[exportnamelen] = '\0';
-        debug ("newstyle negotiation: %s: "
-               "client requested export '%s' (ignored)",
-               optname, requested_exportname);
+        memcpy (conn->exportname, &data[4], exportnamelen);
+        conn->exportname[exportnamelen] = '\0';
+        debug ("newstyle negotiation: %s: client requested export
'%s'",
+               optname, conn->exportname);
 
         /* The spec is confusing, but it is required that we send back
          * NBD_INFO_EXPORT, even if the client did not request it!
diff --git a/server/public.c b/server/public.c
index 630de9b..96ab353 100644
--- a/server/public.c
+++ b/server/public.c
@@ -379,3 +379,16 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
   return 0;
 #endif
 }
+
+const char *
+nbdkit_export_name (void)
+{
+  struct connection *conn = threadlocal_get_conn ();
+
+  if (!conn) {
+    nbdkit_error ("no connection in this thread");
+    return NULL;
+  }
+
+  return conn->exportname;
+}
-- 
2.23.0

In nbdkit API v3 we will probably add the export name as an extra
parameter, but while we are using API v2 we can get the same effect by
calling nbdkit_export_name().
---
 plugins/sh/nbdkit-sh-plugin.pod | 6 ++++--
 plugins/sh/sh.c                 | 6 +++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/plugins/sh/nbdkit-sh-plugin.pod b/plugins/sh/nbdkit-sh-plugin.pod
index 5789908..a088838 100644
--- a/plugins/sh/nbdkit-sh-plugin.pod
+++ b/plugins/sh/nbdkit-sh-plugin.pod
@@ -237,9 +237,11 @@ with status C<1>; unrecognized output is ignored.
 
 =item C<open>
 
- /path/to/script open <readonly>
+ /path/to/script open <readonly> <exportname>
 
-The C<readonly> parameter will be C<true> or C<false>.
+The C<readonly> parameter will be C<true> or C<false>.  The
+C<exportname> parameter, if present, is the export name passed to the
+server from the client.
 
 On success this should print the handle (any string) on stdout and
 exit with code C<0>.  If the handle ends with a newline character then
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index e0d370d..56d30ca 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -324,7 +324,11 @@ sh_open (int readonly)
 {
   char *h = NULL;
   size_t hlen;
-  const char *args[] = { script, "open", readonly ? "true"
: "false", NULL };
+  const char *args[] +    { script, "open",
+      readonly ? "true" : "false",
+      nbdkit_export_name () ? : "",
+      NULL };
 
   /* We store the string returned by open in the handle. */
   switch (call_read (&h, &hlen, args)) {
-- 
2.23.0

---
 tests/Makefile.am         |  3 ++
 tests/test-export-name.sh | 86 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index b5806bb..f54597b 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -395,6 +395,9 @@ endif HAVE_LIBGUESTFS
 # Test export flags.
 TESTS += test-eflags.sh
 
+# Test export name.
+TESTS += test-export-name.sh
+
 # common disk image shared with several tests
 if HAVE_GUESTFISH
 check_DATA += disk
diff --git a/tests/test-export-name.sh b/tests/test-export-name.sh
new file mode 100755
index 0000000..2f63911
--- /dev/null
+++ b/tests/test-export-name.sh
@@ -0,0 +1,86 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright (C) 2019 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 nbdsh --version
+requires stat --version
+
+sock=`mktemp -u`
+files="$sock exportname.pid"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Create a sh plugin which echos the export name back in the device.
+start_nbdkit -P exportname.pid -U $sock \
+             sh - <<'EOF'
+case "$1" in
+    open)
+        h=`mktemp $tmpdir/disk-XXXXXX`
+        echo -n "$3" > $h
+        echo $h
+        ;;
+    get_size)
+        stat -c '%s' "$2"
+        ;;
+    pread)
+        dd if="$2" skip=$4 count=$3 iflag=skip_bytes,count_bytes
+        ;;
+    *) exit 2 ;;
+esac
+EOF
+
+# Try to read back various export names from the plugin.
+for e in "" "test" "/" "//" "
" \
+        
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
+do
+    export e sock
+    nbdsh -c '
+import os
+
+e = os.environ["e"]
+h.set_export_name (e)
+h.connect_unix (os.environ["sock"])
+
+size = h.get_size ()
+print ("size=%r e=%r" % (size, e))
+assert size == len (e)
+
+# Zero-sized reads are not defined in the NBD protocol.
+if size > 0:
+    buf = h.pread (size, 0)
+    print ("buf=%r e=%r" % (buf, e))
+    assert buf.decode() == e
+'
+done
-- 
2.23.0

On 9/12/19 4:01 PM, Richard W.M. Jones wrote:
> ---
>  tests/Makefile.am         |  3 ++
>  tests/test-export-name.sh | 86 +++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
> +# Create a sh plugin which echos the export name back in the device.
echoes? One of those weird words with dual acceptable spellings, where
it depends on who you ask for which variant is preferred (we already
fixed the code base to prefer zeroes over zeros, though).
> +start_nbdkit -P exportname.pid -U $sock \
> +             sh - <<'EOF'
> +case "$1" in
> +    open)
> +        h=`mktemp $tmpdir/disk-XXXXXX`
> +        echo -n "$3" > $h
I prefer printf over echo -n (we require bash which means we are more
portable than a random shell where POSIX says -n is undefined; but you
can still make bash misbehave with shopt -s xpg_echo)

printf %s "%3" > $h
> +        echo $h
> +        ;;
Hmm - instead of making the handle be the name of a temporary file that
contains the export name, you could just make the handle _be_ the export
name:

open)
  printf %s "$3"
  ;;
> +    get_size)
> +        stat -c '%s' "$2"
> +        ;;
at which point, you can simplify to:

get_size)
  echo ${#2}
  ;;
> +    pread)
> +        dd if="$2" skip=$4 count=$3 iflag=skip_bytes,count_bytes
> +        ;;
and this would be something like:

pread)
  echo "$2" | dd skip=$4 count=$3 iflag=skip_bytes,count_bytes
  ;;

or bypass dd by exploiting your knowledge of the client:
pread)
  if test $4.$3 = "0.${#2}"; then
    printf %s "$2"
  else
    echo "EINVAL unexpected alignment" >&2
  fi
  ;;

With those changes, we cut down on the number of fork()s and filesystem
modifications :)
> +    *) exit 2 ;;
> +esac
> +EOF
> +
> +# Try to read back various export names from the plugin.
> +for e in "" "test" "/" "//" "
" \
> +        
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"
> +do
> +    export e sock
> +    nbdsh -c '
> +import os
> +
> +e = os.environ["e"]
> +h.set_export_name (e)
> +h.connect_unix (os.environ["sock"])
Could be done with nbdsh --connect "nbd+unix://$e?socket=$sock".  But
what you did here works as well (--uri would work too once you could
rely on nbdsh 1.2, but for now sticking to --connect is more portable)
> +
> +size = h.get_size ()
> +print ("size=%r e=%r" % (size, e))
Cute test.  Up to you what you want to fold in from my suggestions, but
I think the series is ready.

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