Libguestfs - Sep 2019 - Re: [PATCH nbdkit] server: Add nbdkit_export_name() to allow export name to be read.

This is the sort of thing I had in mind for option (1) here:

  https://www.redhat.com/archives/libguestfs/2019-September/msg00047.html

It does reveal that the way we currently list exports is naive to say
the least ...

Rich.

This allows plugins (or filters) to read the export name which was
passed to the server from the client.

UNFINISHED:

 - Needs tests
---
 TODO                                 |  8 ++++++
 docs/nbdkit-plugin.pod               | 39 ++++++++++++++++++++++++++++
 server/connections.c                 |  1 +
 server/internal.h                    |  1 +
 server/protocol-handshake-newstyle.c | 30 ++++++++++++---------
 server/public.c                      | 10 +++++++
 6 files changed, 77 insertions(+), 12 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..53687ad 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -362,6 +362,45 @@ 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.
+
+This may return C<NULL> which does I<not> indicate an error:
+
+=over 4
+
+=item *
+
+It only returns the export name when there is a connected client.
+
+=item *
+
+If the server is using the oldstyle protocol the client does not send
+an export name.
+
+=back
+
 =head1 CALLBACKS
 
 =head2 C<.name>
diff --git a/server/connections.c b/server/connections.c
index b582764..a1fea54 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -385,6 +385,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 9314e8f..3bfc1a7 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -180,6 +180,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/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 9ddc319..e1301a0 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -274,11 +274,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;
+      }
+      memcpy (conn->exportname, data, optlen+1);
 
       /* We have to finish the handshake by sending handshake_finish. */
       if (finish_newstyle_options (conn, &exportsize) == -1)
@@ -413,19 +419,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..eb7f996 100644
--- a/server/public.c
+++ b/server/public.c
@@ -379,3 +379,13 @@ nbdkit_nanosleep (unsigned sec, unsigned nsec)
   return 0;
 #endif
 }
+
+const char *
+nbdkit_export_name (void)
+{
+  struct connection *conn = threadlocal_get_conn ();
+
+  if (!conn)
+    return NULL;
+  return conn->exportname;
+}
-- 
2.23.0

On 9/10/19 6:20 AM, Richard W.M. Jones wrote:
> This allows plugins (or filters) to read the export name which was
> passed to the server from the client.
> 
> UNFINISHED:
> 
>  - Needs tests
> ---
>  TODO                                 |  8 ++++++
>  docs/nbdkit-plugin.pod               | 39 ++++++++++++++++++++++++++++
>  server/connections.c                 |  1 +
>  server/internal.h                    |  1 +
>  server/protocol-handshake-newstyle.c | 30 ++++++++++++---------
>  server/public.c                      | 10 +++++++
>  6 files changed, 77 insertions(+), 12 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.
See my idea in the other thread for how we could add a callback for
plugins to advertise the list of exports that they want exposed to clients.

> +++ b/docs/nbdkit-plugin.pod
> @@ -362,6 +362,45 @@ 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.
We may want to add text here and/or in .can_multi_conn to clarify that
advertising multi-conn only matters to clients connecting to the SAME
export name; it is safe to advertise multi-conn even when serving
different content to different export names.
> +
> +=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.
> +
> +This may return C<NULL> which does I<not> indicate an error:
> +
> +=over 4
> +
> +=item *
> +
> +It only returns the export name when there is a connected client.
> +
> +=item *
> +
> +If the server is using the oldstyle protocol the client does not send
> +an export name.
Or, we could just blindly state that in oldstyle mode, the client always
behaves as if it connected to the export named "".  After all, qemu
3.1
was where we changed things to allow a client to request an explicit
export name of "" yet still connect to an oldstyle server in that
case.
> +++ b/server/protocol-handshake-newstyle.c
> @@ -274,11 +274,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;
> +      }
> +      memcpy (conn->exportname, data, optlen+1);
Why malloc/memcpy instead of strdup() (two instances)?

Otherwise the idea looks reasonable. I replied in the other thread about
how we could expose this to the sh plugin's open, even if we don't bump
to v3 API yet.

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