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/7822f8fd66f51c730c6b2dc61e1677b4b579e256/server/protocol-handshake-newstyle.c#L242
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/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' 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
signature.asc
Description: OpenPGP digital signature
_______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
