[Libguestfs] [PATCH nbdkit 1/4] Add reflection plugin.

[Eric Blake]

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>