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 <eblake@redhat.com>
---
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 = "";
+ }
+
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) {
--
2.17.1
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 <eblake@redhat.com><mailto:eblake@redhat.com>
---
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 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 <vsementsov@virtuozzo.com><mailto:vsementsov@virtuozzo.com>
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
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 <eblake@redhat.com>
> ---
> 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 <vsementsov@virtuozzo.com>
> 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
On 10/3/18 12:55 PM, 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 <eblake@redhat.com> > --- > 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.