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

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190910/d4e131bc/attachment.sig>