Re: [Qemu-block] [PATCH v3 17/19] qemu-nbd: Add --list option
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, ) < 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
Re: [Qemu-block] [PATCH v3 17/19] qemu-nbd: Add --list option
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, ) < 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
Re: [Qemu-block] [PATCH v3 17/19] qemu-nbd: Add --list option
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 > Message-Id: <20181215135324.152629-21-ebl...@redhat.com> > Reviewed-by: Richard W.M. Jones > > --- > 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
[Qemu-block] [PATCH v3 17/19] qemu-nbd: Add --list option
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 Message-Id: <20181215135324.152629-21-ebl...@redhat.com> Reviewed-by: Richard W.M. Jones --- 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. @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