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 <vsement...@virtuozzo.com> >> >> > -- Best regards, Vladimir
