[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v2] nbd: Flip qemu-nbd to prefer newstyle; add -
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v2] nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior |
Date: |
Wed, 3 Oct 2018 18:32:21 +0000 |
03.10.2018 20:55, Eric Blake wrote:
> Oldstyle NBD negotiation cannot perform any of the extensions that
> we have recently been relying on. While you can always pass -x ""
> to get newstyle negotiation, these days, it is better to just default
> to newstyle (with an empty export name) and fall back to oldstyle
> only on an explicit request.
>
> qemu as client can manage either style since 2.6.0, commit 69b49502d8
>
> For comparison:
>
> nbdkit 1.3 switched its default to newstyle (Jan 2018):
> https://github.com/libguestfs/nbdkit/commit/b2a8aecc
> https://github.com/libguestfs/nbdkit/commit/8158e773
>
> nbd 3.10 dropped oldstyle long ago (Mar 2015):
> https://github.com/NetworkBlockDevice/nbd/commit/36940193
>
> Signed-off-by: Eric Blake <address@hidden>
> ---
> v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'
> ---
> qemu-nbd.c | 37 +++++++++++++++++++++++--------------
> 1 file changed, 23 insertions(+), 14 deletions(-)
>
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index 51b9d38c727..e35c97e7ca4 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -56,7 +56,7 @@
> #define MBR_SIZE 512
>
> static NBDExport *exp;
> -static bool newproto;
> +static bool oldstyle;
> static int verbose;
> static char *srcpath;
> static SocketAddress *saddr;
> @@ -84,8 +84,9 @@ static void usage(const char *name)
> " -e, --shared=NUM device can be shared by NUM clients (default
> '1')\n"
> " -t, --persistent don't exit on the last connection\n"
> " -v, --verbose display extra debugging information\n"
> -" -x, --export-name=NAME expose export by name\n"
> -" -D, --description=TEXT with -x, also export a human-readable
> description\n"
> +" -x, --export-name=NAME expose export by name (default "")\n"
> +" -D, --description=TEXT expose a human-readable description of export\n"
> +" -O, --oldstyle force oldstyle (not with -x, -D, or
> --tls-creds)\n"
> "\n"
> "Exposing part of the image:\n"
> " -o, --offset=OFFSET offset into the image\n"
> @@ -354,7 +355,7 @@ static void nbd_accept(QIONetListener *listener,
> QIOChannelSocket *cioc,
>
> nb_fds++;
> nbd_update_server_watch();
> - nbd_client_new(newproto ? NULL : exp, cioc,
> + nbd_client_new(oldstyle ? exp : NULL, cioc,
> tlscreds, NULL, nbd_client_closed);
> }
>
> @@ -502,7 +503,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:O";
> struct option lopt[] = {
> { "help", no_argument, NULL, 'h' },
> { "version", no_argument, NULL, 'V' },
> @@ -529,6 +530,7 @@ int main(int argc, char **argv)
> { "object", required_argument, NULL, QEMU_NBD_OPT_OBJECT },
> { "export-name", required_argument, NULL, 'x' },
> { "description", required_argument, NULL, 'D' },
> + { "oldstyle", no_argument, NULL, 'O' },
> { "tls-creds", required_argument, NULL, QEMU_NBD_OPT_TLSCREDS },
> { "image-opts", no_argument, NULL, QEMU_NBD_OPT_IMAGE_OPTS },
> { "trace", required_argument, NULL, 'T' },
> @@ -723,6 +725,9 @@ int main(int argc, char **argv)
> case 'D':
> export_description = optarg;
> break;
> + case 'O':
> + oldstyle = true;
> + break;
> case 'v':
> verbose = 1;
> break;
> @@ -799,7 +804,16 @@ int main(int argc, char **argv)
> }
> }
>
> + if (!oldstyle && !export_name) {
> + /* Set the default NBD protocol export name, to favor new style. */
> + export_name = "";
> + }
this
> +
> if (tlscredsid) {
> + if (oldstyle) {
> + error_report("TLS is incompatible with oldstyle");
> + exit(EXIT_FAILURE);
> + }
> if (sockpath) {
> error_report("TLS is only supported with IPv4/IPv6");
> exit(EXIT_FAILURE);
> @@ -808,11 +822,6 @@ int main(int argc, char **argv)
> error_report("TLS is not supported with a host device");
> exit(EXIT_FAILURE);
> }
> - if (!export_name) {
> - /* Set the default NBD protocol export name, since
> - * we *must* use new style protocol for TLS */
> - export_name = "";
> - }
> tlscreds = nbd_get_tls_creds(tlscredsid, &local_err);
> if (local_err) {
> error_report("Failed to get TLS creds %s",
> @@ -1013,13 +1022,13 @@ int main(int argc, char **argv)
> error_report_err(local_err);
> exit(EXIT_FAILURE);
> }
> + if (oldstyle && (export_name || export_description)) {
> + error_report("oldstyle negotiation cannot set export details");
> + exit(EXIT_FAILURE);
> + }
and this are simple option checks, shouldn't it be better to keep them
before more difficult logic, immediately after getopt loop?
with or without this:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> if (export_name) {
> nbd_export_set_name(exp, export_name);
> nbd_export_set_description(exp, export_description);
> - newproto = true;
> - } else if (export_description) {
> - error_report("Export description requires an export name");
> - exit(EXIT_FAILURE);
> }
>
> if (device) {
--
Best regards,
Vladimir