Libguestfs - Sep 2019 - [nbdkit] Access export name from plugins

Of course at the moment nbdkit parses the NBD export name option but
doesn't really do anything with it (except logging it).

I wonder if we should make this available to plugins, in case they
wish to serve different content to different clients based on the
export name.  Note I'm not suggesting that we use this feature in any
existing plugins.

If we wanted to do this there seem like two possible ways to do it:

(1) Add a call, like nbdkit_export_name, which plugins could call from
any connected method to get the current export name, eg:

static void *
myplugin_open (int readonly)
{
  const char *export = nbdkit_export_name ();

  ... Do something based on the export name ...
}

The implementation of this is straightforward.  It simply reads the
exportname global from the server and returns it.  It also doesn't
change the existing API at all.

(2) A better way might be to bump the API version to 3 and that would
allow us to change the open() callback:

static void *
myplugin_open (int readonly, const char *exportname)
{
  ... Do something based on the export name ...
}

This is cleaner but is obviously a larger change.  Also if we were
going to bump the API version I'd want to at the same time do all the
other things we are planning for API 3, and that potentially makes it
a much bigger deal:

https://github.com/libguestfs/nbdkit/blob/b485ade71464fc351828e2fcce54464709bf234d/TODO#L166

It has the advantage of making it easier to access the export name
from sh plugins, since those don't have access to nbdkit_* API calls
(or at least we haven't thought about how we would do that).

Thoughts?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Tue, Sep 10, 2019 at 11:01:10AM +0100, Richard W.M. Jones
wrote:
> The implementation of this is straightforward.  It simply reads the
> exportname global from the server and returns it.  It also doesn't
> change the existing API at all.
Sorry - of course I don't mean returning the global exportname, but
returning the per-thread exportname negotiated for the current
connection.  The implementation is still relatively simply however.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

On 9/10/19 5:01 AM, Richard W.M. Jones wrote:
> Of course at the moment nbdkit parses the NBD export name option but
> doesn't really do anything with it (except logging it).
> 
> I wonder if we should make this available to plugins, in case they
> wish to serve different content to different clients based on the
> export name.  Note I'm not suggesting that we use this feature in any
> existing plugins.
> 
> If we wanted to do this there seem like two possible ways to do it:
at least two ways.
> 
> (1) Add a call, like nbdkit_export_name, which plugins could call from
> any connected method to get the current export name, eg:
> 
> static void *
> myplugin_open (int readonly)
> {
>   const char *export = nbdkit_export_name ();
> 
>   ... Do something based on the export name ...
> }
I would also consider adding an optional callback:

struct nbdkit_export {
  char *name;
  size_t size;
  // maybe other fields for supporting NBD_OPT_INFO...
};

const nbdkit_export *(*list_exports) (void);

A plugin that does not implement the callback advertises whatever export
name was passed on the command line with -e (or defaults to advertising
just ""), and accepts any name, or we could even state that
NBD_OPT_LIST
fails unless the callback is present.

A plugin that DOES implement the callback causes -e to fail (the plugin
is now in charge of the export names, instead of -e), and makes
NBD_OPT_LIST and NBD_OPT_INFO advertise the list returned by
.list_exports.  The plugin is responsible for returning an array of zero
or more structures terminated by an entry where .name is NULL (although
returning zero entries means the plugin is not currently accepting
clients, as no export name from the client will match).  The
.list_exports() callback would be called _prior_ to .open on a
per-connection basis (thus, there is no 'void *handle' parameter,
because the connection has not yet been established; but because it is
called once per connecting client, the resulting list could be
dynamically modified such as providing a list of filenames present in a
directory).

We'd have to figure out lifetimes - would the plugin return a malloc()d
list that nbdkit frees, or would we need a pair of functions, where the
.list_exports function returns a const list, and then nbdkit calls
.free_exports (if defined) to let the plugin clean up any allocations.
(The latter is slightly nicer, in case the plugin is using some other
memory management scheme, such as a language binding, rather than
requiring the copying of data into a form where nbdkit calling free() is
safe).
> 
> The implementation of this is straightforward.  It simply reads the
> exportname global from the server and returns it.  It also doesn't
> change the existing API at all.
> 
> (2) A better way might be to bump the API version to 3 and that would
> allow us to change the open() callback:
> 
> static void *
> myplugin_open (int readonly, const char *exportname)
> {
>   ... Do something based on the export name ...
> }
> 
> This is cleaner but is obviously a larger change.  Also if we were
> going to bump the API version I'd want to at the same time do all the
> other things we are planning for API 3, and that potentially makes it
> a much bigger deal:
> 
>
https://github.com/libguestfs/nbdkit/blob/b485ade71464fc351828e2fcce54464709bf234d/TODO#L166
Yeah, bumping to API 3 is a much bigger deal; we can get most of the
benefits of just letting the plugin know WHICH export name was requested
without making that bump.
> 
> It has the advantage of making it easier to access the export name
> from sh plugins, since those don't have access to nbdkit_* API calls
> (or at least we haven't thought about how we would do that).
Note that with sh plugins, we could still make things work with v2, as
follows:

sh_open() calls nbdkit_export_name() unconditionally, prior to forming
the command line for calling into the script.  Then we alter the command
line we pass to the script:

/path/to/script open <readonly> <exportname>

where the sh plugin always gets to know what export name the client
chose, thanks to the new exportname parameter.  Compatibility-wise,
older scripts fall in one of two categories: either they ignore
unexpected parameters (no change required), or they choke because
exportname was unexpected (but it's a one-line change to the script to
deal with it).  New scripts run against an older nbdkit that did not
provide the exportname parameter will see "$3" as empty (if the script
runs under 'set -u', then spell things "${3-}").

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

On Tue, Sep 10, 2019 at 07:58:49AM -0500, Eric Blake
wrote:
> On 9/10/19 5:01 AM, Richard W.M. Jones wrote:
> > Of course at the moment nbdkit parses the NBD export name option but
> > doesn't really do anything with it (except logging it).
> > 
> > I wonder if we should make this available to plugins, in case they
> > wish to serve different content to different clients based on the
> > export name.  Note I'm not suggesting that we use this feature in
any
> > existing plugins.
> > 
> > If we wanted to do this there seem like two possible ways to do it:
> 
> at least two ways.
> 
> > 
> > (1) Add a call, like nbdkit_export_name, which plugins could call from
> > any connected method to get the current export name, eg:
> > 
> > static void *
> > myplugin_open (int readonly)
> > {
> >   const char *export = nbdkit_export_name ();
> > 
> >   ... Do something based on the export name ...
> > }
> 
> I would also consider adding an optional callback:
> 
> struct nbdkit_export {
>   char *name;
>   size_t size;
>   // maybe other fields for supporting NBD_OPT_INFO...
> };
> 
> const nbdkit_export *(*list_exports) (void);
I see this as needed too, but solving a slightly different problem
from the one which Xiubo Li has.

However there's an interesting point here: Should the list of exports
be handled separately or should it be integrated?

For example, we could imagine first implementing nbdkit_export_name as
I described, and ignoring the NBD_OPT_LIST problem.  Later we decide
to implement NBD_OPT_LIST by adding a .list_exports callback.  But a
(bad) plugin could answer with a completely different list of exports
from what it supports in the .open call.

Or we could try to find a way to tie these two together, but I'm not
clear at the moment how we would do that.

(I'm inclined to think this is over-designing things and it doesn't
matter very much if a plugin gives the wrong answer when asked to list
exports.)

[...]
> Note that with sh plugins, we could still make things work with v2, as
> follows:
> 
> sh_open() calls nbdkit_export_name() unconditionally, prior to forming
> the command line for calling into the script.  Then we alter the command
> line we pass to the script:
> 
> /path/to/script open <readonly> <exportname>
> 
> where the sh plugin always gets to know what export name the client
> chose, thanks to the new exportname parameter.  Compatibility-wise,
> older scripts fall in one of two categories: either they ignore
> unexpected parameters (no change required), or they choke because
> exportname was unexpected (but it's a one-line change to the script to
> deal with it).  New scripts run against an older nbdkit that did not
> provide the exportname parameter will see "$3" as empty (if the
script
> runs under 'set -u', then spell things "${3-}").
sh plugins really should ignore extra parameters they don't know
about.  We should probably say that in the man page.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-df lists disk usage of guests without needing to install any
software inside the virtual machine.  Supports Linux and Windows.
http://people.redhat.com/~rjones/virt-df/