5:51 a.m.
On 9/17/19 2:42 AM, Richard W.M. Jones wrote:
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...
I didn't actually check the definition of MAX_OPTION_LENGTH, assuming it
was the same as our 64M limit on other traffic. But yes, now that I
checked that it is defined at 4k, including overhead, I concur that it
does limit nbdkit to less than 4k as written (we could enlarge that
limit if we wanted to cater to more corner cases of compliant client
requests).
>
> 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'
Or even:
( # subshell to allow us to alter the parent with exec
nbdkit --exit-with-parent ... &
exec nbdsh ...
)
>> +#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.
I didn't see '(default exportname)' in git; could be a followup if we
still want it.
>> +
>> +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.
I just realized "%%" might be another useful test for potentially
problematic names (to ensure we aren't accidentally passing the string
directly through printf)
> 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.
I've hit it before (such as nbdkit-nbd-plugin, which is why I had to add
the 'retry=N' parameter), or when testing other things (such as
nbdkit-truncate-filter rounding up into a size that is impossible).
I'll probably play with that more today.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org