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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5839 bytes --]

On 1/17/19 4:05 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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).
>>

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

A start of that experiment has now been posted:
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg04196.html


>>   @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.

I'm thinking of adding this (and see conversation below that mentions [1]):

diff --git i/qemu-nbd.texi w/qemu-nbd.texi
index 65caeb7874a..0d297eed6db 100644
--- i/qemu-nbd.texi
+++ w/qemu-nbd.texi
@@ -105,7 +105,9 @@ 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.
+a remote NBD server.  This enables list mode, and is incompatible
+with options that change behavior related to a specific export (such as
+@option{--export-name}, @option{--offset}, ...).
 @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


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

Will do. Probably leftovers from earlier attempts as I changed my
approach over time.


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

Which, as the commit message mentions, is for servers so old and rare
that it really doesn't matter.

>   ...
> }
> 
> Why not to print @size for example, if @flags field has a bug?
> 
> Or, then, why to print flags, if @size has a bug?

Because we don't have to worry about those servers being mainstream.

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

I don't mind adding in any more options that you think are useful to
flag the user on.  Looks like I missed seen_aio, seen_cache,
seen_discard.  Catching '-s' is harder, as it merely sets a bit within
flags rather than a witness variable.

> Also, I think, it would be good to specify in Usage
> (or in man), which options are available for list mode.

I worry that keeping an exact list may be a maintenance nightmare (the
two are likely to get out of sync); does my proposed wording above at
[1] satisfy the problem by at least making the user aware that not all
combinations will work?

Another alternative would be to just silently ignore all per-export
options, instead of warning the user that they are incompatible.  I
don't know if that's any friendlier, but it is less code.

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

That could be a separate patch, if it is desired (or squashed into 2/19)

> 
> 
> 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>
> 
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]