[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 17/19] qemu-nbd: Add --list option
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 17/19] qemu-nbd: Add --list option |
Date: |
Thu, 17 Jan 2019 10:58:00 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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
address@hidden, @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 <address@hidden>
>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature