[Libguestfs] [PATCH nbdkit 1/4] Add reflection plugin.

[Richard W.M. Jones]

On Mon, Sep 16, 2019 at 10:33:18AM -0500, Eric Blake wrote:
> Is it worth noting that the NBD protocol imposes a 4k limit on the
> export name, which would limit things to about a 3k disk image when
> using base64?  (It looks like nbdkit does not currently enforce that
> limit -- maybe it should, but as a separate patch -- but even if we
> don't change that, you're still limited to finding a client willing to
> send an export name larger than the protocol permits)

Yes the documentation should say this, I will modify it (and an
equivalent change in libnbd).

As for nbdkit I did check the code actually before posting and my
impression is that it does enforce the limit to a little under
MAX_OPTION_LENGTH (4096) albeit a indirectly.  Don't these lines limit
it?

https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b4b579e256/server/protocol-handshake-newstyle.c#L242
https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b4b579e256/server/protocol-handshake-newstyle.c#L272
https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b4b579e256/server/protocol-handshake-newstyle.c#L401


> > +The plugin only supports read-only access.  To make the disk writable,
> > +add L<nbdkit-cow-filter(1)> on top.
> > +
> > +=head1 EXAMPLES
> > +
> > +Create a reflection disk.  By setting the export name to C<"hello">
> > +when we open it, a virtual disk of only 5 bytes containing these
> > +characters is created.  We then display the contents:
> > +
> > + $ nbdkit reflection mode=exportname
> > + $ nbdsh -u 'nbd://localhost/hello' -c - <<'EOF'
> > + size = h.get_size()
> > + print("size = %d" % size)
> > + buf = h.pread(size, 0)
> > + print("buf = %r" % buf)
> > + EOF
> 
> This leaves nbdkit running as a background daemon after the fact.  Is it
> worth implementing a new 'nbdkit --one-shot' flag that causes nbdkit to
> exit after the first client disconnects?

https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b4b579e256/TODO#L11

The problem is that some clients connect twice - libguestfs in
particular does this (because it runs qemu-img info first).  That
makes implementing something which will be useful rather impractical,
and it's probably better to use captive nbdkit here.

This works better IMO:

$ nbdkit --exit-with-parent reflection mode=exportname &
$ nbdsh -u 'nbd://localhost/hello' -c - <<'EOF'
...

[...]

> > +=head1 SEE ALSO
> > +
> > +L<nbdkit(1)>,
> > +L<nbdkit-plugin(3)>,
> > +L<nbdkit-cow-filter(1)>,
> > +L<nbdkit-data-plugin(1)>,
> > +L<nbdkit-memory-plugin(1)/Preloading small amounts of data>.
> 
> Except that nbdkit-memory-plugin points only to nbdkit-data-plugin, so
> is this link helping?

Hmmm true :-/  Will remove it.

> > +static int
> > +reflection_config (const char *key, const char *value)
> > +{
> > +  if (strcmp (key, "mode") == 0) {
> > +    if (strcasecmp (value, "exportname") == 0) {
> 
> Do we want to be nice and allow 'export-name' as an [undocumented]
> alternative spelling?

OK.

> > +      mode = MODE_EXPORTNAME;
> > +    }
> > +    else if (strcasecmp (value, "base64exportname") == 0) {
> 
> similarly for 'base64-export-name'

OK.

> 
> > +#define reflection_config_help \
> > +  "mode=MODE    Plugin mode."
> > +
> 
> Worth listing the valid values of MODE, or the fact that this parameter
> is optional because it defaults to exportname?

OK.

> > +++ b/tests/test-reflection-base64.sh
> 
> > +
> > +requires nbdsh --version
> > +# XXX This needs to test $PYTHON somehow.
> > +#requires python -c 'import base64'
> 
> Not just any python, but the version of python hard-coded as what will
> run nbdsh (which may be different than the $PYTHON that nbdkit was
> compiled with).  I'm not sure what to suggest, other than just:
> 
> requires nbdsh -c 'import base64'
> 
> which could, in fact, be a single test (no need to test if nbdsh
> --version works if nbdsh -c ... works).

Yes, that's a much better idea.

> 
> > +
> > +export e sock
> > +for e in "" "test" "テスト" \
> 
> Worth also testing '-n' or '\n' (two strings that caused problems with
> the sh plugin implementation) (here, and in the plaintext version)?

I see that you have implemented this for tests/test-export-name.c so I
will modify this test in the same way.

> > +
> > +# Test that it fails if the caller passes in non-base64 data.  The
> > +# server drops the connection in this case so it's not very graceful
> > +# but we should at least get an nbd.Error and not something else.
> > +nbdsh -c '
> > +import os
> > +import sys
> > +
> > +h.set_export_name ("xyz")
> > +try:
> > +    h.connect_unix (os.environ["sock"])
> > +    # This should not happen.
> > +    sys.exit (1)
> > +except nbd.Error as ex:
> > +    sys.exit (0)
> > +# This should not happen.
> > +sys.exit (1)
> 
> The test looks good.  (Yeah, figuring out how to make it more graceful
> in the future might be nice, but that's not this patch's problem. I'm
> thinking that if a plugin's .open() fails, nbdkit could reply with
> NBD_REP_ERR_UNKNOWN or NBD_REP_ERR_INVALID, and then wait for the client
> to disconnect, rather than the current hard hangup initiated by the server)

Abrupt disconnection isn't very good here.  I guess we have never
really thought before about .open failing.

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