2:42 a.m.
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/7822f8fd66f51c730c6b2dc61e1677b...
https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b...
https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b...
> +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/7822f8fd66f51c730c6b2dc61e1677b...
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