Libguestfs - Sep 2019 - Re: [PATCH nbdkit 1/4] Add reflection plugin.

On 9/15/19 9:55 AM, Richard W.M. Jones wrote:
> The source for the easter egg example is not included, but it is:
> 
>         org 0x7c00
>         ;  clear screen
>         mov ah,0
>         mov al,3
>         int 0x10
>         ; print string
>         mov ah,0x13
>         mov bl,0xa
>         mov al,1
>         mov cx,len
>         mov dh,0
>         mov dl,0
>         mov bp,hello
>         int 0x10
>         hlt
> hello:  db "*** Hello from nbdkit! ***",0xd,0xa
> len:    equ $-hello
>         ; pad to end of sector
>         times 510-($-$$) db 0
>         ; boot signature
>         db 0x55,0xaa
> ---
> +++ b/plugins/reflection/nbdkit-reflection-plugin.pod
> @@ -0,0 +1,104 @@
> +=head1 NAME
> +
> +nbdkit-reflection-plugin - reflect export name back to the client
> +
> +=head1 SYNOPSIS
> +
> + nbdkit reflection [mode=]exportname|base64exportname
> +
> +=head1 DESCRIPTION
> +
> +C<nbdkit-reflection-plugin> is a test plugin which reflects
> +information sent by the client back to the client.
> +
> +In its default mode (C<mode=exportname>) it converts the export name
> +passed from the client into a disk image.  C<mode=base64exportname>
is
> +similar except the client must base64-encode the data in the export
> +name, allowing arbitrary binary data to be sent (see L</EXAMPLES>
> +below to make this clearer).
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)
> +
> +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?  I also wonder if we want to
add a way to differentiate between SIGINT (terminate nbdkit as soon as
possible) vs. SIGHUP (no new clients permitted, but keep nbdkit up and
running for as long as current clients stay connected) [it would have to
be a new command line option, as existing users are expecting SIGHUP and
SIGINT to behave identically, and the idea is somewhat at odds with our
other idea in TODO of having the v3 protocol have strongly-typed plugin
parameters with a way to send SIGHUP as a way to do a live reload of
parameters backed by a file]

> +=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?
> +++ b/plugins/reflection/reflection.c
> +/* The mode. */
> +enum mode {
> +  MODE_EXPORTNAME,
> +  MODE_BASE64EXPORTNAME,
> +};
> +static enum mode mode = MODE_EXPORTNAME;
Explicit assignment to a 0 value, instead of relying on .bss; but I'm
okay with it (it's not as obvious as =0 or =false).
> +
> +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?
> +      mode = MODE_EXPORTNAME;
> +    }
> +    else if (strcasecmp (value, "base64exportname") == 0) {
similarly for 'base64-export-name'

> +#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?

> +/* Create the per-connection handle.
> + *
> + * This is a rather unusual plugin because it has to parse data sent
> + * by the client.  For security reasons, be careful about:
> + *
> + * - Returning more data than is sent by the client.
> + *
> + * - Inputs that result in unbounded output.
> + *
> + * - Inputs that could hang, crash or exploit the server.
> + */
Nice reminder. I agree that we're okay for now, but as we add new modes
in the future, it will remind us to think about it.

> +/* Read-only plugin so multi-conn is safe. */
> +static int
> +reflection_can_multi_conn (void *handle)
> +{
> +  return 1;
Correct for now.  I also see your followup to patch 4 that changes this
once IP addresses are exposed.

> +++ 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).

> +
> +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)?
> +
> +# 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)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

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
https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b4b579e256/server/protocol-handshake-newstyle.c#L272
https://github.com/libguestfs/nbdkit/blob/7822f8fd66f51c730c6b2dc61e1677b4b579e256/server/protocol-handshake-newstyle.c#L401

> > +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/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'
...

[...]
> > +=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

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