Re: [Qemu-devel] [PATCH v3 17/19] qemu-nbd: Add --list option

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

12.01.2019 20:58, Eric Blake wrote:
> We want to be able to detect whether a given qemu NBD server is
> exposing the right export(s) and dirty bitmaps, at least for
> regression testing.  We could use 'nbd-client -l' from the upstream
> NBD project to list exports, but it's annoying to rely on
> out-of-tree binaries; furthermore, nbd-client doesn't necessarily
> know about all of the qemu NBD extensions.  Thus, it is time to add
> a new mode to qemu-nbd that merely sniffs all possible information
> from the server during handshake phase, then disconnects and dumps
> the information.
> 
> This patch actually implements --list/-L, while reusing other
> options such as --tls-creds for now designating how to connect
> as the client (rather than their non-list usage of how to operate
> as the server).
> 
> I debated about adding this functionality to something akin to
> 'qemu-img info' - but that tool does not readily lend itself
> to connecting to an arbitrary NBD server without also tying to
> a specific export (I may, however, still add ImageInfoSpecificNBD
> for reporting the bitmaps available when connecting to a single
> export).  And, while it may feel a bit odd that normally
> qemu-nbd is a server but 'qemu-nbd -L' is a client, we are not
> really making the qemu-nbd binary that much larger, because
> 'qemu-nbd -c' has to operate as both server and client
> simultaneously across two threads when feeding the kernel module
> for /dev/nbdN access.
> 
> Sample output:
> $ qemu-nbd -L
> exports available: 1
>   export: ''
>    size:  65536
>    flags: 0x4ed ( flush fua trim zeroes df cache )
>    min block: 512
>    opt block: 4096
>    max block: 33554432
>    available meta contexts: 1
>     base:allocation
> 
> Note that the output only lists sizes if the server sent
> NBD_FLAG_HAS_FLAGS, because a newstyle server does not give
> the size otherwise.  It has the side effect that for really
> old servers that did not send any flags, the size is not
> output even though it was available.  However, I'm not too
> concerned about that - oldstyle servers are (rightfully)
> getting less common to encounter (qemu 3.0 was the last
> version where we even serve it), and most existing servers
> that still even offer oldstyle negotiation (such as nbdkit)
> still send flags (since that was added to the NBD protocol
> in 2007 to permit read-only connections).
> 
> Not done here, but maybe worth future experiments: capture
> the meat of NBDExportInfo into a QAPI struct, and use the
> generated QAPI pretty-printers instead of hand-rolling our
> output loop.  It would also permit us to add a JSON output
> mode for machine parsing.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> Message-Id: <20181215135324.152629-21-eblake@redhat.com>
> Reviewed-by: Richard W.M. Jones <rjones@redhat.com>
> 
> ---
> v3: comment tweak [Rich], rebase to earlier changes
> ---
>   qemu-nbd.texi |  27 +++++++--
>   qemu-nbd.c    | 155 +++++++++++++++++++++++++++++++++++++++++++++-----
>   2 files changed, 165 insertions(+), 17 deletions(-)
> 
> diff --git a/qemu-nbd.texi b/qemu-nbd.texi
> index 3f22559beb4..65caeb7874a 100644
> --- a/qemu-nbd.texi
> +++ b/qemu-nbd.texi
> @@ -2,6 +2,8 @@
>   @c man begin SYNOPSIS
>   @command{qemu-nbd} [OPTION]... @var{filename}
> 
> +@command{qemu-nbd} @option{-L} [OPTION]...
> +
>   @command{qemu-nbd} @option{-d} @var{dev}
>   @c man end
>   @end example
> @@ -14,6 +16,8 @@ Other uses:
>   @itemize
>   @item
>   Bind a /dev/nbdX block device to a QEMU server (on Linux).
> +@item
> +As a client to query exports of a remote NBD server.
>   @end itemize
> 
>   @c man end
> @@ -31,13 +35,15 @@ See the @code{qemu(1)} manual page for full details of the properties
>   supported. The common object types that it makes sense to define are the
>   @code{secret} object, which is used to supply passwords and/or encryption
>   keys, and the @code{tls-creds} object, which is used to supply TLS
> -credentials for the qemu-nbd server.
> +credentials for the qemu-nbd server or client.
>   @item -p, --port=@var{port}
> -The TCP port to listen on (default @samp{10809}).
> +The TCP port to listen on as a server, or connect to as a client
> +(default @samp{10809}).
>   @item -o, --offset=@var{offset}
>   The offset into the image.
>   @item -b, --bind=@var{iface}
> -The interface to bind to (default @samp{0.0.0.0}).
> +The interface to bind to as a server, or connect to as a client
> +(default @samp{0.0.0.0}).
>   @item -k, --socket=@var{path}
>   Use a unix socket with path @var{path}.
>   @item --image-opts
> @@ -97,10 +103,14 @@ Set the NBD volume export name (default of a zero-length string).
>   @item -D, --description=@var{description}
>   Set the NBD volume export description, as a human-readable
>   string.
> +@item -L, --list
> +Connect as a client and list all details about the exports exposed by
> +a remote NBD server.
>   @item --tls-creds=ID
>   Enable mandatory TLS encryption for the server by setting the ID
>   of the TLS credentials object previously created with the --object
> -option.
> +option; or provide the credentials needed for connecting as a client
> +in list mode.

may be "list mode (--list)", as "list mode" is not directly defined. On the other hand,
list option is extremely close to tls-creds, so it is obvious anyway.

>   @item --fork
>   Fork off the server process and exit the parent once the server is running.
>   @item -v, --verbose
> @@ -159,6 +169,15 @@ qemu-nbd -c /dev/nbd0 -f qcow2 file.qcow2
>   qemu-nbd -d /dev/nbd0
>   @end example
> 
> +Query a remote server to see details about what export(s) it is
> +serving on port 10809, and authenticating via PSK:
> +
> +@example
> +qemu-nbd \
> +  --object tls-creds-psk,id=tls0,dir=/tmp/keys,username=eblake,endpoint=client \
> +  --tls-creds tls0 -L -b remote.example.com
> +@end example
> +
>   @c man end
> 
>   @ignore
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index f1c24683129..daccb86d0d7 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -76,7 +76,8 @@ static void usage(const char *name)
>   {
>       (printf) (
>   "Usage: %s [OPTIONS] FILE\n"
> -"QEMU Disk Network Block Device Server\n"
> +"  or:  %s -L [OPTIONS]\n"
> +"QEMU Disk Network Block Device Utility\n"
>   "\n"
>   "  -h, --help                display this help and exit\n"
>   "  -V, --version             output version information and exit\n"
> @@ -98,6 +99,7 @@ static void usage(const char *name)
>   "  -B, --bitmap=NAME         expose a persistent dirty bitmap\n"
>   "\n"
>   "General purpose options:\n"
> +"  -L, --list                list exports available from another NBD server\n"
>   "  --object type,id=ID,...   define an object such as 'secret' for providing\n"
>   "                            passwords and/or encryption keys\n"
>   "  --tls-creds=ID            use id of an earlier --object to provide TLS\n"
> @@ -131,7 +133,7 @@ static void usage(const char *name)
>   "      --image-opts          treat FILE as a full set of image options\n"
>   "\n"
>   QEMU_HELP_BOTTOM "\n"
> -    , name, NBD_DEFAULT_PORT, "DEVICE");
> +    , name, name, NBD_DEFAULT_PORT, "DEVICE");
>   }
> 
>   static void version(const char *name)
> @@ -243,6 +245,92 @@ static void termsig_handler(int signum)
>   }
> 
> 
> +static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls,
> +                                const char *hostname)
> +{
> +    int ret = EXIT_FAILURE;
> +    int rc;
> +    Error *err = NULL;
> +    QIOChannelSocket *sioc;
> +    NBDExportInfo *list;
> +    int i, j;
> +
> +    sioc = qio_channel_socket_new();
> +    if (qio_channel_socket_connect_sync(sioc, saddr, &err) < 0) {
> +        error_report_err(err);
> +        goto out;

May be just return EXIT_FAUILURE here;
remove out label;
s/out_socket/out

> +    }
> +    rc = nbd_receive_export_list(QIO_CHANNEL(sioc), tls, hostname, &list,
> +                                 &err);
> +    if (rc < 0) {
> +        if (err) {
> +            error_report_err(err);
> +        }
> +        goto out_socket;
> +    }
> +    printf("exports available: %d\n", rc);
> +    for (i = 0; i < rc; i++) {
> +        printf(" export: '%s'\n", list[i].name);
> +        if (list[i].description && *list[i].description) {
> +            printf("  description: %s\n", list[i].description);
> +        }
> +        if (list[i].flags & NBD_FLAG_HAS_FLAGS) {

actually this is

if (server not have a bug of not setting NBD_FLAG_HAS_FLAGS) {
  ...
}

Why not to print @size for example, if @flags field has a bug?

Or, then, why to print flags, if @size has a bug?

> +            printf("  size:  %" PRIu64 "\n", list[i].size);
> +            printf("  flags: 0x%x (", list[i].flags);
> +            if (list[i].flags & NBD_FLAG_READ_ONLY) {
> +                printf(" readonly");
> +            }
> +            if (list[i].flags & NBD_FLAG_SEND_FLUSH) {
> +                printf(" flush");
> +            }
> +            if (list[i].flags & NBD_FLAG_SEND_FUA) {
> +                printf(" fua");
> +            }
> +            if (list[i].flags & NBD_FLAG_ROTATIONAL) {
> +                printf(" rotational");
> +            }
> +            if (list[i].flags & NBD_FLAG_SEND_TRIM) {
> +                printf(" trim");
> +            }
> +            if (list[i].flags & NBD_FLAG_SEND_WRITE_ZEROES) {
> +                printf(" zeroes");
> +            }
> +            if (list[i].flags & NBD_FLAG_SEND_DF) {
> +                printf(" df");
> +            }
> +            if (list[i].flags & NBD_FLAG_CAN_MULTI_CONN) {
> +                printf(" multi");
> +            }
> +            if (list[i].flags & NBD_FLAG_SEND_RESIZE) {
> +                printf(" resize");
> +            }
> +            if (list[i].flags & NBD_FLAG_SEND_CACHE) {
> +                printf(" cache");
> +            }
> +            printf(" )\n");
> +        }
> +        if (list[i].min_block) {
> +            printf("  min block: %u\n", list[i].min_block);
> +            printf("  opt block: %u\n", list[i].opt_block);
> +            printf("  max block: %u\n", list[i].max_block);
> +        }
> +        if (list[i].n_contexts) {
> +            printf("  available meta contexts: %d\n", list[i].n_contexts);
> +            for (j = 0; j < list[i].n_contexts; j++) {
> +                printf("   %s\n", list[i].contexts[j]);
> +            }
> +        }
> +    }
> +    nbd_free_export_list(list, rc);
> +
> +    ret = EXIT_SUCCESS;
> + out_socket:
> +    object_unref(OBJECT(sioc));
> + out:
> +    return ret;
> +}
> +
> +
>   #if HAVE_NBD_DEVICE
>   static void *show_parts(void *arg)
>   {
> @@ -425,7 +513,8 @@ static QemuOptsList qemu_object_opts = {
> 
> 
> 
> -static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
> +static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, bool list,
> +                                          Error **errp)
>   {
>       Object *obj;
>       QCryptoTLSCreds *creds;
> @@ -445,10 +534,18 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp)
>           return NULL;
>       }
> 
> -    if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
> -        error_setg(errp,
> -                   "Expecting TLS credentials with a server endpoint");
> -        return NULL;
> +    if (list) {
> +        if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
> +            error_setg(errp,
> +                       "Expecting TLS credentials with a client endpoint");
> +            return NULL;
> +        }
> +    } else {
> +        if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
> +            error_setg(errp,
> +                       "Expecting TLS credentials with a server endpoint");
> +            return NULL;
> +        }
>       }
>       object_ref(obj);
>       return creds;
> @@ -471,7 +568,8 @@ static void setup_address_and_port(const char **address, const char **port)
>   static const char *socket_activation_validate_opts(const char *device,
>                                                      const char *sockpath,
>                                                      const char *address,
> -                                                   const char *port)
> +                                                   const char *port,
> +                                                   bool list)
>   {
>       if (device != NULL) {
>           return "NBD device can't be set when using socket activation";
> @@ -489,6 +587,10 @@ static const char *socket_activation_validate_opts(const char *device,
>           return "TCP port number can't be set when using socket activation";
>       }
> 
> +    if (list) {
> +        return "List mode is incompatible with socket activation";
> +    }
> +
>       return NULL;
>   }
> 
> @@ -512,7 +614,7 @@ int main(int argc, char **argv)
>       int64_t fd_size;
>       QemuOpts *sn_opts = NULL;
>       const char *sn_id_or_name = NULL;
> -    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:";
> +    const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:B:L";
>       struct option lopt[] = {
>           { "help", no_argument, NULL, 'h' },
>           { "version", no_argument, NULL, 'V' },
> @@ -525,6 +627,7 @@ int main(int argc, char **argv)
>           { "bitmap", required_argument, NULL, 'B' },
>           { "connect", required_argument, NULL, 'c' },
>           { "disconnect", no_argument, NULL, 'd' },
> +        { "list", no_argument, NULL, 'L' },
>           { "snapshot", no_argument, NULL, 's' },
>           { "load-snapshot", required_argument, NULL, 'l' },
>           { "nocache", no_argument, NULL, 'n' },
> @@ -559,7 +662,7 @@ int main(int argc, char **argv)
>       Error *local_err = NULL;
>       BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
>       QDict *options = NULL;
> -    const char *export_name = ""; /* Default export name */
> +    const char *export_name = NULL; /* defaults to "" later for server mode */
>       const char *export_description = NULL;
>       const char *bitmap = NULL;
>       const char *tlscredsid = NULL;
> @@ -567,6 +670,7 @@ int main(int argc, char **argv)
>       bool writethrough = true;
>       char *trace_file = NULL;
>       bool fork_process = false;
> +    bool list = false;
>       int old_stderr = -1;
>       unsigned socket_activation;
> 
> @@ -760,13 +864,32 @@ int main(int argc, char **argv)
>           case QEMU_NBD_OPT_FORK:
>               fork_process = true;
>               break;
> +        case 'L':
> +            list = true;
> +            break;
>           }
>       }
> 
> -    if ((argc - optind) != 1) {
> +    if (list) {
> +        if (argc != optind) {
> +            error_report("List mode is incompatible with a file name");
> +            exit(EXIT_FAILURE);
> +        }
> +        if (export_name || export_description || dev_offset || partition ||
> +            device || disconnect || fmt || sn_id_or_name || bitmap) {
> +            error_report("List mode is incompatible with per-device settings");
> +            exit(EXIT_FAILURE);

and what about aio, discard, etc? Also, I think, it would be good to specify in Usage
(or in man), which options are available for list mode.

Hm, note, --help has grouping of options and man don't.

> +        }
> +        if (fork_process) {
> +            error_report("List mode is incompatible with forking");
> +            exit(EXIT_FAILURE);
> +        }
> +    } else if ((argc - optind) != 1) {
>           error_report("Invalid number of arguments");
>           error_printf("Try `%s --help' for more information.\n", argv[0]);
>           exit(EXIT_FAILURE);
> +    } else if (!export_name) {
> +        export_name = "";
>       }
> 
>       qemu_opts_foreach(&qemu_object_opts,
> @@ -785,7 +908,8 @@ int main(int argc, char **argv)
>       } else {
>           /* Using socket activation - check user didn't use -p etc. */
>           const char *err_msg = socket_activation_validate_opts(device, sockpath,
> -                                                              bindto, port);
> +                                                              bindto, port,
> +                                                              list);
>           if (err_msg != NULL) {
>               error_report("%s", err_msg);
>               exit(EXIT_FAILURE);
> @@ -808,7 +932,7 @@ int main(int argc, char **argv)
>               error_report("TLS is not supported with a host device");
>               exit(EXIT_FAILURE);
>           }
> -        tlscreds = nbd_get_tls_creds(tlscredsid, &local_err);
> +        tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err);
>           if (local_err) {
>               error_report("Failed to get TLS creds %s",
>                            error_get_pretty(local_err));
> @@ -816,6 +940,11 @@ int main(int argc, char **argv)
>           }
>       }
> 
> +    if (list) {
> +        saddr = nbd_build_socket_address(sockpath, bindto, port);
> +        return qemu_nbd_client_list(saddr, tlscreds, bindto);
> +    }
> +
>   #if !HAVE_NBD_DEVICE
>       if (disconnect || device) {
>           error_report("Kernel /dev/nbdN support not available");
> 


I don't have good understanding of tls related things, the rest looks OK,
my suggestions are optional, so, if you don't want to improve docs and
option conflict checking now:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


-- 
Best regards,
Vladimir