nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

PATCH In review Eric Blake 2 participants 4 messages Oct 3, 2018

Share & tools

Eric Blake

author

2018-10-03 17:55 UTC #0 lore raw

PATCH v2 1/1 nbd: Flip qemu-nbd to prefer newstyle; add -O for old behavior

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 <[email protected]>

---
v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'

---
 qemu-nbd.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

qemu-nbd.c +23 ~~-14~~

index 51b9d38c727..e35c97e7ca4 100644
@@ -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 = "";
+    }
+
     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);
+    }
     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) {

Signature · 1 line

2.17.1

Vladimir Sementsov-Ogievskiy

Reviewed-by on v2 1/1

2018-10-03 18:36 UTC #1 lore raw

Quoted 86 lines · Eric Blake

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 <[email protected]>

---
v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'

---
  qemu-nbd.c | 37 +++++++++++++++++++++++--------------
  1 file changed, 23 insertions(+), 14 deletions(-)

qemu-nbd.c +10 -5

index 51b9d38c727..e35c97e7ca4 100644
@@ -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;

qemu-nbd.c int main(int argc, char **argv) · line 810

Quoted 7 lines

qemu-nbd.c +4 -0

@@ -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

Quoted 22 lines

qemu-nbd.c +5 -5

+
      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",

qemu-nbd.c int main(int argc, char **argv) · line 1028

Quoted 7 lines

qemu-nbd.c +4 -0

@@ -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 <[email protected]>

Quoted 10 lines

qemu-nbd.c +0 -4

      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) {

Signature · 2 lines

Best regards,
Vladimir

Vladimir Sementsov-Ogievskiy

Reviewed-by on v2 1/1

2018-10-03 18:37 UTC #2 lore raw

In reply to Eric Blake

03.10.2018 20:55, Eric Blake wrote:

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 <[email protected]><mailto:[email protected]>

---
v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'

---
 qemu-nbd.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

qemu-nbd.c +14 -5

index 51b9d38c727..e35c97e7ca4 100644
@@ -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 very simple option checks, so, it is better to place them together and after getopt loop, before the other more difficult logic.

but it's a nit-picking, so, with or without:

Reviewed-by Vladimir Sementsov-Ogievskiy <[email protected]><mailto:[email protected]>

     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

Eric Blake

author

2018-10-03 20:43 UTC #3 lore raw

In reply to Eric Blake

Quoted 17 lines · Eric Blake

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

Quoted 6 lines

Signed-off-by Eric Blake <[email protected]>
---
v2: improve 'qemu-nbd --help', drop redundant variable 'newproto'
---
  qemu-nbd.c | 37 +++++++++++++++++++++++--------------
  1 file changed, 23 insertions(+), 14 deletions(-)

But I still forgot to document it in qemu-nbd.texi.

I'm dropping this patch in favor of Vladimir's stronger series, now that Rich has agreed with my observation that nbdkit can bridge the needs of any non-qemu client that still only talks oldstyle.

Signature · 3 lines

Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org