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

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

17.01.2019 19:58, Eric Blake wrote:
> 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?

I don't really care of it, current version is OK too. Of course, an addition
sounds better than nothing)

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


-- 
Best regards,
Vladimir