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 _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
