| Message ID | 20181130220344.3350618-14-eblake@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | nbd: add qemu-nbd --list | expand |
On Fri, Nov 30, 2018 at 04:03:42PM -0600, 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, we plan on adding > 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 > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > qemu-nbd.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 141 insertions(+), 12 deletions(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index c57053a0795..e19a841b869 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -76,6 +76,7 @@ static void usage(const char *name) > { > (printf) ( > "Usage: %s [OPTIONS] FILE\n" > +" or: %s -L [OPTIONS]\n" > "QEMU Disk Network Block Device Server\n" > "\n" > " -h, --help display this help and exit\n" > @@ -97,6 +98,7 @@ static void usage(const char *name) > " -P, --partition=NUM only expose partition NUM\n" > "\n" > "General purpose options:\n" > +" -L, --list list NBD exports visible to client\n" > " --object type,id=ID,... define an object such as 'secret' for providing\n" > " passwords and/or encryption keys\n" > " --tls-creds=ID use id of an earlier --object to provide TLS\n" > @@ -130,7 +132,7 @@ static void usage(const char *name) > " --image-opts treat FILE as a full set of image options\n" > "\n" > QEMU_HELP_BOTTOM "\n" > - , name, NBD_DEFAULT_PORT, "DEVICE"); > + , name, name, NBD_DEFAULT_PORT, "DEVICE"); > } > > static void version(const char *name) > @@ -242,6 +244,92 @@ static void termsig_handler(int signum) > } > > > +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; > + } > + rc = nbd_receive_export_list(QIO_CHANNEL(sioc), tls, hostname, &list, > + &err); > + if (rc < 0) { > + if (err) { > + error_report_err(err); > + } > + goto out_socket; > + } > + 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) { > + printf(" size: %" PRIu64 "\n", list[i].size); > + printf(" flags: 0x%x (", list[i].flags); > + if (list[i].flags & NBD_FLAG_READ_ONLY) { > + printf(" readonly"); > + } This adds an extra space in the output unconditionally, if that's a problem, ie. the output looks like: flags: 0x61 ( trim zeroes ) instead of: flags: 0x61 (trim zeroes) if that's a problem. I might have used a loop with a static array of flag names, like: const char *nbd_flag_name[] = { ... .5 = "trim", .6 = "zeroes", }; although it's a bit awkward given the current definitions in include/block/nbd.h. Anyway not a big issue. > + if (list[i].flags & NBD_FLAG_SEND_FLUSH) { > + printf(" flush"); > + } > + if (list[i].flags & NBD_FLAG_SEND_FUA) { > + printf(" fua"); > + } > + if (list[i].flags & NBD_FLAG_ROTATIONAL) { > + printf(" rotational"); > + } > + if (list[i].flags & NBD_FLAG_SEND_TRIM) { > + printf(" trim"); > + } > + if (list[i].flags & NBD_FLAG_SEND_WRITE_ZEROES) { > + printf(" zeroes"); > + } > + if (list[i].flags & NBD_FLAG_SEND_DF) { > + printf(" df"); > + } > + if (list[i].flags & NBD_FLAG_CAN_MULTI_CONN) { > + printf(" multi"); > + } > + if (list[i].flags & NBD_FLAG_SEND_RESIZE) { > + printf(" resize"); > + } > + if (list[i].flags & NBD_FLAG_SEND_CACHE) { > + printf(" cache"); > + } > + printf(" )\n"); > + } > + if (list[i].min_block) { > + printf(" min block: %u\n", list[i].min_block); > + printf(" opt block: %u\n", list[i].opt_block); > + printf(" max block: %u\n", list[i].max_block); > + } > + if (list[i].n_contexts) { > + printf(" available meta contexts: %d\n", list[i].n_contexts); > + for (j = 0; j < list[i].n_contexts; j++) { > + printf(" %s\n", list[i].contexts[j]); > + } > + } > + } > + nbd_free_export_list(list, rc); > + > + ret = EXIT_SUCCESS; > + out_socket: > + object_unref(OBJECT(sioc)); > + out: > + return ret; > +} > + > + > #if HAVE_NBD_DEVICE > static void *show_parts(void *arg) > { > @@ -424,7 +512,8 @@ static QemuOptsList qemu_object_opts = { > > > > -static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) > +static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, bool list, > + Error **errp) > { > Object *obj; > QCryptoTLSCreds *creds; > @@ -444,10 +533,18 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) > return NULL; > } > > - if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) { > - error_setg(errp, > - "Expecting TLS credentials with a server endpoint"); > - return NULL; > + if (list) { > + if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) { > + error_setg(errp, > + "Expecting TLS credentials with a client endpoint"); > + return NULL; > + } > + } else { > + if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) { > + error_setg(errp, > + "Expecting TLS credentials with a server endpoint"); > + return NULL; > + } > } > object_ref(obj); > return creds; > @@ -470,7 +567,8 @@ static void setup_address_and_port(const char **address, const char **port) > static const char *socket_activation_validate_opts(const char *device, > const char *sockpath, > const char *address, > - const char *port) > + const char *port, > + bool list) > { > if (device != NULL) { > return "NBD device can't be set when using socket activation"; > @@ -488,6 +586,10 @@ static const char *socket_activation_validate_opts(const char *device, > return "TCP port number can't be set when using socket activation"; > } > > + if (list) { > + return "List mode is incompatible with socket activation"; > + } > + > return NULL; > } > > @@ -511,7 +613,7 @@ int main(int argc, char **argv) > off_t fd_size; > QemuOpts *sn_opts = NULL; > const char *sn_id_or_name = NULL; > - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:"; > + const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:L"; > struct option lopt[] = { > { "help", no_argument, NULL, 'h' }, > { "version", no_argument, NULL, 'V' }, > @@ -523,6 +625,7 @@ int main(int argc, char **argv) > { "partition", required_argument, NULL, 'P' }, > { "connect", required_argument, NULL, 'c' }, > { "disconnect", no_argument, NULL, 'd' }, > + { "list", no_argument, NULL, 'L' }, > { "snapshot", no_argument, NULL, 's' }, > { "load-snapshot", required_argument, NULL, 'l' }, > { "nocache", no_argument, NULL, 'n' }, > @@ -558,13 +661,14 @@ int main(int argc, char **argv) > Error *local_err = NULL; > BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF; > QDict *options = NULL; > - const char *export_name = ""; /* Default export name */ > + const char *export_name = NULL; > const char *export_description = NULL; > const char *tlscredsid = NULL; > bool imageOpts = false; > bool writethrough = true; > char *trace_file = NULL; > bool fork_process = false; > + bool list = false; > int old_stderr = -1; > unsigned socket_activation; > > @@ -764,13 +868,32 @@ int main(int argc, char **argv) > case QEMU_NBD_OPT_FORK: > fork_process = true; > break; > + case 'L': > + list = true; > + break; > } > } > > - 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) { > + error_report("List mode is incompatible with per-device settings"); > + exit(EXIT_FAILURE); > + } > + if (fork_process) { > + error_report("List mode is incompatible with forking"); > + exit(EXIT_FAILURE); > + } > + } else if ((argc - optind) != 1) { > error_report("Invalid number of arguments"); > error_printf("Try `%s --help' for more information.\n", argv[0]); > exit(EXIT_FAILURE); > + } else if (!export_name) { > + export_name = ""; > } > > qemu_opts_foreach(&qemu_object_opts, > @@ -789,7 +912,8 @@ int main(int argc, char **argv) > } else { > /* Using socket activation - check user didn't use -p etc. */ > const char *err_msg = socket_activation_validate_opts(device, sockpath, > - bindto, port); > + bindto, port, > + list); > if (err_msg != NULL) { > error_report("%s", err_msg); > exit(EXIT_FAILURE); > @@ -812,7 +936,7 @@ int main(int argc, char **argv) > error_report("TLS is not supported with a host device"); > exit(EXIT_FAILURE); > } > - tlscreds = nbd_get_tls_creds(tlscredsid, &local_err); > + tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err); > if (local_err) { > error_report("Failed to get TLS creds %s", > error_get_pretty(local_err)); > @@ -820,6 +944,11 @@ int main(int argc, char **argv) > } > } > > + if (list) { > + saddr = nbd_build_socket_address(sockpath, bindto, port); > + return qemu_nbd_client_list(saddr, tlscreds, bindto); > + } > + > if (disconnect) { > #if HAVE_NBD_DEVICE > int nbdfd = open(argv[optind], O_RDWR); The code is a bit awkward since we've now effectively added a new mode to qemu-nbd. I can foresee in future that someone will add some code before the ‘if (list) / if (disconnect)’ statements which will apply only to "server mode", and they won't necessarily know that they have to add a mode check to that code, resulting in the list option doing something weird that might not show up as broken in unit tests. Still, I don't have any idea how to make it better, for all the reasons you outlined in your commit message. I'll leave it to others who know qemu code in greater detail to comment. Rich.
01.12.2018 1:03, 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, we plan on adding ha, in this patch, not plan but 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 don't you plan to bind this all to QAPI and expose in json? > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > qemu-nbd.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 141 insertions(+), 12 deletions(-) > > diff --git a/qemu-nbd.c b/qemu-nbd.c > index c57053a0795..e19a841b869 100644 > --- a/qemu-nbd.c > +++ b/qemu-nbd.c > @@ -76,6 +76,7 @@ static void usage(const char *name) > { > (printf) ( > "Usage: %s [OPTIONS] FILE\n" > +" or: %s -L [OPTIONS]\n" > "QEMU Disk Network Block Device Server\n" Do anyone know, why thunderbird add additional space to the lines started from space when quoting, which breaks indentation in quoted patches? How to fix it? I use plain text. > "\n" > " -h, --help display this help and exit\n" > @@ -97,6 +98,7 @@ static void usage(const char *name) > " -P, --partition=NUM only expose partition NUM\n" > "\n" > "General purpose options:\n" > +" -L, --list list NBD exports visible to client\n" hm. I think, user who things that qemu-nbd is only a server, can understand this as "dry run, don't actually start the server, but only list exports, which will be available to clients, keeping in mind all other specified options". so, may be, "list remote NBD server exports and options" or something like this? > " --object type,id=ID,... define an object such as 'secret' for providing\n" > " passwords and/or encryption keys\n" > " --tls-creds=ID use id of an earlier --object to provide TLS\n" > @@ -130,7 +132,7 @@ static void usage(const char *name) > " --image-opts treat FILE as a full set of image options\n" > "\n" > QEMU_HELP_BOTTOM "\n" > - , name, NBD_DEFAULT_PORT, "DEVICE"); > + , name, name, NBD_DEFAULT_PORT, "DEVICE"); > } > > static void version(const char *name) > @@ -242,6 +244,92 @@ static void termsig_handler(int signum) > } > > > +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; > + } > + rc = nbd_receive_export_list(QIO_CHANNEL(sioc), tls, hostname, &list, > + &err); > + if (rc < 0) { > + if (err) { > + error_report_err(err); > + } > + goto out_socket; > + } > + 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) { > + printf(" size: %" PRIu64 "\n", list[i].size); size is available only if NBD_FLAG_HAS_FLAGS? at least, accordingly to code, in case 0: size is unrelated to flags. > + printf(" flags: 0x%x (", list[i].flags); > + if (list[i].flags & NBD_FLAG_READ_ONLY) { > + printf(" readonly"); > + } > + if (list[i].flags & NBD_FLAG_SEND_FLUSH) { > + printf(" flush"); > + } > + if (list[i].flags & NBD_FLAG_SEND_FUA) { > + printf(" fua"); > + } > + if (list[i].flags & NBD_FLAG_ROTATIONAL) { > + printf(" rotational"); > + } > + if (list[i].flags & NBD_FLAG_SEND_TRIM) { > + printf(" trim"); > + } > + if (list[i].flags & NBD_FLAG_SEND_WRITE_ZEROES) { > + printf(" zeroes"); > + } > + if (list[i].flags & NBD_FLAG_SEND_DF) { > + printf(" df"); > + } > + if (list[i].flags & NBD_FLAG_CAN_MULTI_CONN) { > + printf(" multi"); > + } > + if (list[i].flags & NBD_FLAG_SEND_RESIZE) { > + printf(" resize"); > + } > + if (list[i].flags & NBD_FLAG_SEND_CACHE) { > + printf(" cache"); > + } > + printf(" )\n"); > + } > + if (list[i].min_block) { > + printf(" min block: %u\n", list[i].min_block); > + printf(" opt block: %u\n", list[i].opt_block); > + printf(" max block: %u\n", list[i].max_block); > + } > + if (list[i].n_contexts) { > + printf(" available meta contexts: %d\n", list[i].n_contexts); > + for (j = 0; j < list[i].n_contexts; j++) { > + printf(" %s\n", list[i].contexts[j]); > + } > + } > + } > + nbd_free_export_list(list, rc); > + > + ret = EXIT_SUCCESS; > + out_socket: > + object_unref(OBJECT(sioc)); > + out: > + return ret; > +} > +
On 12/7/18 6:48 AM, Vladimir Sementsov-Ogievskiy wrote: > 01.12.2018 1:03, 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, we plan on adding > > ha, in this patch, not plan but add:) Yep, copy-and-paste of a common prefix across multiple patches. I'll probably let the text diverge in v2 to be a bit more accurate per-patch, at the loss of the nice copy-and-paste. >> 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 > > don't you plan to bind this all to QAPI and expose in json? No. As explained above, QAPI is very much centered on per-BDS actions, while this action is a per-server action (where many servers have only one export, but it is also possible to have a server with 0 exports or a plurality of exports). I _can't_ fit this into query-block's ImageInfo details about a block device, because we don't have a way of stating 'connect to this arbitrary server, create an unspecified number of block devices, tell me about each of them, and then throw them all away because we only wanted the info about what the server made available'. The same is true for gluster or other remote access block devices - the QMP commands pre-suppose you already know WHICH specific resource you are accessing, rather than providing a way for you to query the remote server about ALL resources available but without actually selecting those resources. I'm open to ideas about a new QMP command to do such a query, but who would be the client? A management app that wants to hotplug a new NBD device to a running guest can run 'qemu-nbd --list' just as easily as they could run a new QMP command to learn what the server is offering. Without a strong reason of a client that would need this in QMP, I don't see the point in adding it to the qemu binary. >> +++ b/qemu-nbd.c >> @@ -76,6 +76,7 @@ static void usage(const char *name) >> { >> (printf) ( >> "Usage: %s [OPTIONS] FILE\n" >> +" or: %s -L [OPTIONS]\n" >> "QEMU Disk Network Block Device Server\n" > > Do anyone know, why thunderbird add additional space to the lines started from space when quoting, > which breaks indentation in quoted patches? How to fix it? I use plain text. It's a known thunderbird display bug (or more specifically, something that thunderbird does to avoid even worse whitespace corruption later in its pipeline). It shows up in your reply window, but NOT in the recipients' view. I've been dealing with bad formatting in thunderbird for years now, and this latest bug of displaying too many spaces on quoted reply lines that begin with whitespace (which in turn makes lines starting with - or + appear indented wrong) is certainly less annoying than their previous bug of converting all leading whitespace in the quoted reply to a single space. > >> "\n" >> " -h, --help display this help and exit\n" >> @@ -97,6 +98,7 @@ static void usage(const char *name) >> " -P, --partition=NUM only expose partition NUM\n" >> "\n" >> "General purpose options:\n" >> +" -L, --list list NBD exports visible to client\n" > > hm. I think, user who things that qemu-nbd is only a server, can understand this as > "dry run, don't actually start the server, but only list exports, which will be > available to clients, keeping in mind all other specified options". > > so, may be, "list remote NBD server exports and options" or something like this? There's line lengths to worry about, but yes, I agree that adding something to make it a bit more obvious that this option is special is worth the word-smithing attempts. >> + 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) { >> + printf(" size: %" PRIu64 "\n", list[i].size); > > size is available only if NBD_FLAG_HAS_FLAGS? at least, accordingly to code, > in case 0: size is unrelated to flags. case 0: code is for oldstyle servers. There are fewer and fewer of those even worth worrying about (I used nbdkit -o to test that part of my code, and we recently ripped oldstyle out of the qemu server); the ones that remain happen to set NBD_FLAG_HAS_FLAGS even for oldstyle. I'm not too worried if our --list output fails to list size for an oldstyle server that did not set NBD_FLAG_HAS_FLAGS. For all other servers, NBD_FLAG_HAS_FLAGS is a good witness of whether NBD_OPT_INFO was recognized, and there, size is indeed not present unless NBD_OPT_INFO worked.
07.12.2018 18:36, Eric Blake wrote: > On 12/7/18 6:48 AM, Vladimir Sementsov-Ogievskiy wrote: >> 01.12.2018 1:03, 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, we plan on adding >> >> ha, in this patch, not plan but add:) > > Yep, copy-and-paste of a common prefix across multiple patches. I'll probably let the text diverge in v2 to be a bit more accurate per-patch, at the loss of the nice copy-and-paste. > >>> 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 >> >> don't you plan to bind this all to QAPI and expose in json? > > No. As explained above, QAPI is very much centered on per-BDS actions, while this action is a per-server action (where many servers have only one export, but it is also possible to have a server with 0 exports or a plurality of exports). I _can't_ fit this into query-block's ImageInfo details about a block device, because we don't have a way of stating 'connect to this arbitrary server, create an unspecified number of block devices, tell me about each of them, and then throw them all away because we only wanted the info about what the server made available'. The same is true for gluster or other remote access block devices - the QMP commands pre-suppose you already know WHICH specific resource you are accessing, rather than providing a way for you to query the remote server about ALL resources available but without actually selecting those resources. > > I'm open to ideas about a new QMP command to do such a query, but who would be the client? A management app that wants to hotplug a new NBD device to a running guest can run 'qemu-nbd --list' just as easily as they could run a new QMP command to learn what the server is offering. Without a strong reason of a client that would need this in QMP, I don't see the point in adding it to the qemu binary. I didn't mean QMP. For example, QAPI struct ImageCheck is used only in qemu-img, to format output. Anyway, creating a struct in QAPI for something we want to export is a good thing, I think. Also, if, as you said, some management app wants to query this information, again strictly defined data + json output should be a good option. And, if there would be such users, we'll need to track compatibility of exported structure between qemu versions and this is easier with QAPI defined structure. And then, defined structure may be then (at least partly) shared with ImageInfoSpecificNBD. And if we will need at some point a qmp command like query-nbd-server, to get same information through current qmp-connection, not running additional nbd-client, it would be a simple thing to do. > > >>> +++ b/qemu-nbd.c >>> @@ -76,6 +76,7 @@ static void usage(const char *name) >>> { >>> (printf) ( >>> "Usage: %s [OPTIONS] FILE\n" >>> +" or: %s -L [OPTIONS]\n" >>> "QEMU Disk Network Block Device Server\n" >> >> Do anyone know, why thunderbird add additional space to the lines started from space when quoting, >> which breaks indentation in quoted patches? How to fix it? I use plain text. > > It's a known thunderbird display bug (or more specifically, something that thunderbird does to avoid even worse whitespace corruption later in its pipeline). It shows up in your reply window, but NOT in the recipients' view. I've been dealing with bad formatting in thunderbird for years now, and this latest bug of displaying too many spaces on quoted reply lines that begin with whitespace (which in turn makes lines starting with - or + appear indented wrong) is certainly less annoying than their previous bug of converting all leading whitespace in the quoted reply to a single space. > >> >>> "\n" >>> " -h, --help display this help and exit\n" >>> @@ -97,6 +98,7 @@ static void usage(const char *name) >>> " -P, --partition=NUM only expose partition NUM\n" >>> "\n" >>> "General purpose options:\n" >>> +" -L, --list list NBD exports visible to client\n" >> >> hm. I think, user who things that qemu-nbd is only a server, can understand this as >> "dry run, don't actually start the server, but only list exports, which will be >> available to clients, keeping in mind all other specified options". >> >> so, may be, "list remote NBD server exports and options" or something like this? > > There's line lengths to worry about, but yes, I agree that adding something to make it a bit more obvious that this option is special is worth the word-smithing attempts. > >>> + 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) { >>> + printf(" size: %" PRIu64 "\n", list[i].size); >> >> size is available only if NBD_FLAG_HAS_FLAGS? at least, accordingly to code, >> in case 0: size is unrelated to flags. > > case 0: code is for oldstyle servers. There are fewer and fewer of those even worth worrying about (I used nbdkit -o to test that part of my code, and we recently ripped oldstyle out of the qemu server); the ones that remain happen to set NBD_FLAG_HAS_FLAGS even for oldstyle. I'm not too worried if our --list output fails to list size for an oldstyle server that did not set NBD_FLAG_HAS_FLAGS. > > For all other servers, NBD_FLAG_HAS_FLAGS is a good witness of whether NBD_OPT_INFO was recognized, and there, size is indeed not present unless NBD_OPT_INFO worked. >
07.12.2018 18:36, Eric Blake wrote: > On 12/7/18 6:48 AM, Vladimir Sementsov-Ogievskiy wrote: >> 01.12.2018 1:03, 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, we plan on adding >> >> ha, in this patch, not plan but add:) > > Yep, copy-and-paste of a common prefix across multiple patches. I'll probably let the text diverge in v2 to be a bit more accurate per-patch, at the loss of the nice copy-and-paste. > >>> 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 >> >> don't you plan to bind this all to QAPI and expose in json? > > No. As explained above, QAPI is very much centered on per-BDS actions, while this action is a per-server action (where many servers have only one export, but it is also possible to have a server with 0 exports or a plurality of exports). I _can't_ fit this into query-block's ImageInfo details about a block device, because we don't have a way of stating 'connect to this arbitrary server, create an unspecified number of block devices, tell me about each of them, and then throw them all away because we only wanted the info about what the server made available'. The same is true for gluster or other remote access block devices - the QMP commands pre-suppose you already know WHICH specific resource you are accessing, rather than providing a way for you to query the remote server about ALL resources available but without actually selecting those resources. > > I'm open to ideas about a new QMP command to do such a query, but who would be the client? A management app that wants to hotplug a new NBD device to a running guest can run 'qemu-nbd --list' just as easily as they could run a new QMP command to learn what the server is offering. Without a strong reason of a client that would need this in QMP, I don't see the point in adding it to the qemu binary. I didn't mean QMP. For example, QAPI struct ImageCheck is used only in qemu-img, to format output. Anyway, creating a struct in QAPI for something we want to export is a good thing, I think. Also, if, as you said, some management app wants to query this information, again strictly defined data + json output should be a good option. And, if there would be such users, we'll need to track compatibility of exported structure between qemu versions and this is easier with QAPI defined structure. And then, defined structure may be then (at least partly) shared with ImageInfoSpecificNBD. And if we will need at some point a qmp command like query-nbd-server, to get same information through current qmp-connection, not running additional nbd-client, it would be a simple thing to do. > > >>> +++ b/qemu-nbd.c >>> @@ -76,6 +76,7 @@ static void usage(const char *name) >>> { >>> (printf) ( >>> "Usage: %s [OPTIONS] FILE\n" >>> +" or: %s -L [OPTIONS]\n" >>> "QEMU Disk Network Block Device Server\n" >> >> Do anyone know, why thunderbird add additional space to the lines started from space when quoting, >> which breaks indentation in quoted patches? How to fix it? I use plain text. > > It's a known thunderbird display bug (or more specifically, something that thunderbird does to avoid even worse whitespace corruption later in its pipeline). It shows up in your reply window, but NOT in the recipients' view. I've been dealing with bad formatting in thunderbird for years now, and this latest bug of displaying too many spaces on quoted reply lines that begin with whitespace (which in turn makes lines starting with - or + appear indented wrong) is certainly less annoying than their previous bug of converting all leading whitespace in the quoted reply to a single space. > >> >>> "\n" >>> " -h, --help display this help and exit\n" >>> @@ -97,6 +98,7 @@ static void usage(const char *name) >>> " -P, --partition=NUM only expose partition NUM\n" >>> "\n" >>> "General purpose options:\n" >>> +" -L, --list list NBD exports visible to client\n" >> >> hm. I think, user who things that qemu-nbd is only a server, can understand this as >> "dry run, don't actually start the server, but only list exports, which will be >> available to clients, keeping in mind all other specified options". >> >> so, may be, "list remote NBD server exports and options" or something like this? > > There's line lengths to worry about, but yes, I agree that adding something to make it a bit more obvious that this option is special is worth the word-smithing attempts. > >>> + 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) { >>> + printf(" size: %" PRIu64 "\n", list[i].size); >> >> size is available only if NBD_FLAG_HAS_FLAGS? at least, accordingly to code, >> in case 0: size is unrelated to flags. > > case 0: code is for oldstyle servers. There are fewer and fewer of those even worth worrying about (I used nbdkit -o to test that part of my code, and we recently ripped oldstyle out of the qemu server); the ones that remain happen to set NBD_FLAG_HAS_FLAGS even for oldstyle. I'm not too worried if our --list output fails to list size for an oldstyle server that did not set NBD_FLAG_HAS_FLAGS. > > For all other servers, NBD_FLAG_HAS_FLAGS is a good witness of whether NBD_OPT_INFO was recognized, and there, size is indeed not present unless NBD_OPT_INFO worked. >
On 12/7/18 10:49 AM, Vladimir Sementsov-Ogievskiy wrote: >>>> $ 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 >>> >>> don't you plan to bind this all to QAPI and expose in json? >> >> No. As explained above, QAPI is very much centered on per-BDS actions, while this action is a per-server action (where many servers have only one export, but it is also possible to have a server with 0 exports or a plurality of exports). I _can't_ fit this into query-block's ImageInfo details about a block device, because we don't have a way of stating 'connect to this arbitrary server, create an unspecified number of block devices, tell me about each of them, and then throw them all away because we only wanted the info about what the server made available'. The same is true for gluster or other remote access block devices - the QMP commands pre-suppose you already know WHICH specific resource you are accessing, rather than providing a way for you to query the remote server about ALL resources available but without actually selecting those resources. >> >> I'm open to ideas about a new QMP command to do such a query, but who would be the client? A management app that wants to hotplug a new NBD device to a running guest can run 'qemu-nbd --list' just as easily as they could run a new QMP command to learn what the server is offering. Without a strong reason of a client that would need this in QMP, I don't see the point in adding it to the qemu binary. > > I didn't mean QMP. For example, QAPI struct ImageCheck is used only in qemu-img, to format output. > Anyway, creating a struct in QAPI for something we want to export is a good thing, I think. Oh, I see where you're going with this. Just as 'qemu-img info' has routines to pretty-print a QAPI structure (and thus adding ImageInfo to the .json files automatically gets output in HMP without any additional work), you're suggesting that NBDExportInfo be converted into a QAPI struct, even if it won't be tied to QMP, in order to make the output more programmatic instead of manual effort. I'll have to play with that, although it might be a separate series on top of this. > Also, if, as you said, some management app wants to query this information, again strictly > defined data + json output should be a good option. And, if there would be such users, we'll > need to track compatibility of exported structure between qemu versions and this is easier > with QAPI defined structure. > > And then, defined structure may be then (at least partly) shared with ImageInfoSpecificNBD. > > And if we will need at some point a qmp command like query-nbd-server, to get same information through > current qmp-connection, not running additional nbd-client, it would be a simple thing to do. If we ever need future extensions, we'll want to have QAPI structs in place. But whether we implement the QAPI structs now, or at the time of the future extension, is an engineering tradeoff (how much technical debt are we incurring by not doing it now; and how likely are we to ever want the future extension).
diff --git a/qemu-nbd.c b/qemu-nbd.c index c57053a0795..e19a841b869 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -76,6 +76,7 @@ static void usage(const char *name) { (printf) ( "Usage: %s [OPTIONS] FILE\n" +" or: %s -L [OPTIONS]\n" "QEMU Disk Network Block Device Server\n" "\n" " -h, --help display this help and exit\n" @@ -97,6 +98,7 @@ static void usage(const char *name) " -P, --partition=NUM only expose partition NUM\n" "\n" "General purpose options:\n" +" -L, --list list NBD exports visible to client\n" " --object type,id=ID,... define an object such as 'secret' for providing\n" " passwords and/or encryption keys\n" " --tls-creds=ID use id of an earlier --object to provide TLS\n" @@ -130,7 +132,7 @@ static void usage(const char *name) " --image-opts treat FILE as a full set of image options\n" "\n" QEMU_HELP_BOTTOM "\n" - , name, NBD_DEFAULT_PORT, "DEVICE"); + , name, name, NBD_DEFAULT_PORT, "DEVICE"); } static void version(const char *name) @@ -242,6 +244,92 @@ static void termsig_handler(int signum) } +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; + } + rc = nbd_receive_export_list(QIO_CHANNEL(sioc), tls, hostname, &list, + &err); + if (rc < 0) { + if (err) { + error_report_err(err); + } + goto out_socket; + } + 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) { + printf(" size: %" PRIu64 "\n", list[i].size); + printf(" flags: 0x%x (", list[i].flags); + if (list[i].flags & NBD_FLAG_READ_ONLY) { + printf(" readonly"); + } + if (list[i].flags & NBD_FLAG_SEND_FLUSH) { + printf(" flush"); + } + if (list[i].flags & NBD_FLAG_SEND_FUA) { + printf(" fua"); + } + if (list[i].flags & NBD_FLAG_ROTATIONAL) { + printf(" rotational"); + } + if (list[i].flags & NBD_FLAG_SEND_TRIM) { + printf(" trim"); + } + if (list[i].flags & NBD_FLAG_SEND_WRITE_ZEROES) { + printf(" zeroes"); + } + if (list[i].flags & NBD_FLAG_SEND_DF) { + printf(" df"); + } + if (list[i].flags & NBD_FLAG_CAN_MULTI_CONN) { + printf(" multi"); + } + if (list[i].flags & NBD_FLAG_SEND_RESIZE) { + printf(" resize"); + } + if (list[i].flags & NBD_FLAG_SEND_CACHE) { + printf(" cache"); + } + printf(" )\n"); + } + if (list[i].min_block) { + printf(" min block: %u\n", list[i].min_block); + printf(" opt block: %u\n", list[i].opt_block); + printf(" max block: %u\n", list[i].max_block); + } + if (list[i].n_contexts) { + printf(" available meta contexts: %d\n", list[i].n_contexts); + for (j = 0; j < list[i].n_contexts; j++) { + printf(" %s\n", list[i].contexts[j]); + } + } + } + nbd_free_export_list(list, rc); + + ret = EXIT_SUCCESS; + out_socket: + object_unref(OBJECT(sioc)); + out: + return ret; +} + + #if HAVE_NBD_DEVICE static void *show_parts(void *arg) { @@ -424,7 +512,8 @@ static QemuOptsList qemu_object_opts = { -static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) +static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, bool list, + Error **errp) { Object *obj; QCryptoTLSCreds *creds; @@ -444,10 +533,18 @@ static QCryptoTLSCreds *nbd_get_tls_creds(const char *id, Error **errp) return NULL; } - if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) { - error_setg(errp, - "Expecting TLS credentials with a server endpoint"); - return NULL; + if (list) { + if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) { + error_setg(errp, + "Expecting TLS credentials with a client endpoint"); + return NULL; + } + } else { + if (creds->endpoint != QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) { + error_setg(errp, + "Expecting TLS credentials with a server endpoint"); + return NULL; + } } object_ref(obj); return creds; @@ -470,7 +567,8 @@ static void setup_address_and_port(const char **address, const char **port) static const char *socket_activation_validate_opts(const char *device, const char *sockpath, const char *address, - const char *port) + const char *port, + bool list) { if (device != NULL) { return "NBD device can't be set when using socket activation"; @@ -488,6 +586,10 @@ static const char *socket_activation_validate_opts(const char *device, return "TCP port number can't be set when using socket activation"; } + if (list) { + return "List mode is incompatible with socket activation"; + } + return NULL; } @@ -511,7 +613,7 @@ int main(int argc, char **argv) off_t fd_size; QemuOpts *sn_opts = NULL; const char *sn_id_or_name = NULL; - const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:"; + const char *sopt = "hVb:o:p:rsnP:c:dvk:e:f:tl:x:T:D:L"; struct option lopt[] = { { "help", no_argument, NULL, 'h' }, { "version", no_argument, NULL, 'V' }, @@ -523,6 +625,7 @@ int main(int argc, char **argv) { "partition", required_argument, NULL, 'P' }, { "connect", required_argument, NULL, 'c' }, { "disconnect", no_argument, NULL, 'd' }, + { "list", no_argument, NULL, 'L' }, { "snapshot", no_argument, NULL, 's' }, { "load-snapshot", required_argument, NULL, 'l' }, { "nocache", no_argument, NULL, 'n' }, @@ -558,13 +661,14 @@ int main(int argc, char **argv) Error *local_err = NULL; BlockdevDetectZeroesOptions detect_zeroes = BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF; QDict *options = NULL; - const char *export_name = ""; /* Default export name */ + const char *export_name = NULL; const char *export_description = NULL; const char *tlscredsid = NULL; bool imageOpts = false; bool writethrough = true; char *trace_file = NULL; bool fork_process = false; + bool list = false; int old_stderr = -1; unsigned socket_activation; @@ -764,13 +868,32 @@ int main(int argc, char **argv) case QEMU_NBD_OPT_FORK: fork_process = true; break; + case 'L': + list = true; + break; } } - 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) { + error_report("List mode is incompatible with per-device settings"); + exit(EXIT_FAILURE); + } + if (fork_process) { + error_report("List mode is incompatible with forking"); + exit(EXIT_FAILURE); + } + } else if ((argc - optind) != 1) { error_report("Invalid number of arguments"); error_printf("Try `%s --help' for more information.\n", argv[0]); exit(EXIT_FAILURE); + } else if (!export_name) { + export_name = ""; } qemu_opts_foreach(&qemu_object_opts, @@ -789,7 +912,8 @@ int main(int argc, char **argv) } else { /* Using socket activation - check user didn't use -p etc. */ const char *err_msg = socket_activation_validate_opts(device, sockpath, - bindto, port); + bindto, port, + list); if (err_msg != NULL) { error_report("%s", err_msg); exit(EXIT_FAILURE); @@ -812,7 +936,7 @@ int main(int argc, char **argv) error_report("TLS is not supported with a host device"); exit(EXIT_FAILURE); } - tlscreds = nbd_get_tls_creds(tlscredsid, &local_err); + tlscreds = nbd_get_tls_creds(tlscredsid, list, &local_err); if (local_err) { error_report("Failed to get TLS creds %s", error_get_pretty(local_err)); @@ -820,6 +944,11 @@ int main(int argc, char **argv) } } + if (list) { + saddr = nbd_build_socket_address(sockpath, bindto, port); + return qemu_nbd_client_list(saddr, tlscreds, bindto); + } + if (disconnect) { #if HAVE_NBD_DEVICE int nbdfd = open(argv[optind], O_RDWR);
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, we plan on adding 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 Signed-off-by: Eric Blake <eblake@redhat.com> --- qemu-nbd.c | 153 ++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 141 insertions(+), 12 deletions(-)