[Libguestfs] [PATCH nbdkit 1/4] Add reflection plugin.
Eric Blake
eblake at redhat.com
Tue Sep 17 10:51:15 UTC 2019
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20190917/46e0ea8f/attachment.sig>
More information about the Libguestfs
mailing list