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

[Eric Blake]

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

-------------- 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/20190916/3218acf8/attachment.sig>