nbd server: drop old-style negotiation
cover nbd server: drop old-style negotiation
It's unexpected behavior that without -x option qemu-nbd do old-style negotiation. Let's use "" as a default name instead (as it is already done if tls is used) and therefore, drop old-style negotiation from Qemu NBD server.
Vladimir Sementsov-Ogievskiy (2):
qemu-nbd: drop old-style negotiation
nbd/server: drop old-style negotiation
include/block/nbd.h | 3 +-- blockdev-nbd.c | 2 +- nbd/server.c | 53 +++++++++++++-------------------------------- qemu-nbd.c | 25 +++++---------------- 4 files changed, 23 insertions(+), 60 deletions(-)
Signature · 1 line
2.18.0
PATCH 1/2 qemu-nbd: drop old-style negotiation
Use new-style negotiation always, with default "" (empty) export name if it is not specified with '-x' option.
--- qemu-nbd.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
qemu-nbd.c +6 -19
@@ -56,7 +56,6 @@#define MBR_SIZE 512static NBDExport *exp; -static bool newproto;static int verbose;static char *srcpath;static SocketAddress *saddr;
@@ -84,8 +83,8 @@ 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 is empty string)\n" +" -D, --description=TEXT export a human-readable description\n" "\n" "Exposing part of the image:\n" " -o, --offset=OFFSET offset into the image\n"
@@ -354,8 +353,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, nb_fds++;nbd_update_server_watch (); -nbd_client_new (newproto ?NULL : exp, cioc, - tlscreds,NULL , nbd_client_closed); +nbd_client_new (NULL , cioc, tlscreds,NULL , nbd_client_closed); }static void nbd_update_server_watch (void )
@@ -549,7 +547,7 @@ 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 =NULL ; +const char *export_name ="" ;/* Default export name */ const char *export_description =NULL ;const char *tlscredsid =NULL ;bool imageOpts = false;
@@ -808,11 +806,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,14 +1006,8 @@ int main(int argc, char **argv)error_report_err (local_err);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 ); - } +nbd_export_set_name (exp, export_name); +nbd_export_set_description (exp, export_description);if (device) {int ret;
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH 1/2] qemu-nbd: drop old-style negotiation
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
Use new-style negotiation always, with default "" (empty) export name if it is not specified with '-x' option.
If we like this approach (over mine of keeping oldstyle, but via an explicit -O option), then this commit message should add the following:
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 Vladimir Sementsov-Ogievskiy <[email protected]>--- qemu-nbd.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
If we like your patch over mine,
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
PATCH 2/2 nbd/server: drop old-style negotiation
In reply to Vladimir Sementsov-OgievskiyAfter the previous commit, nbd_client_new first parameter is always NULL. Let's drop it with all corresponding old-style negotiation code path which is unreachable now.
--- include/block/nbd.h | 3 +-- blockdev-nbd.c | 2 +- nbd/server.c | 53 +++++++++++++-------------------------------- qemu-nbd.c | 2 +- 4 files changed, 18 insertions(+), 42 deletions(-)
include/block/nbd.h +1 -2
@@ -308,8 +308,7 @@ void nbd_export_set_name(NBDExport *exp, const char *name);void nbd_export_set_description (NBDExport *exp,const char *description);void nbd_export_close_all (void ); -void nbd_client_new (NBDExport *exp, -QIOChannelSocket *sioc, +void nbd_client_new (QIOChannelSocket *sioc,QCryptoTLSCreds *tlscreds,const char *tlsaclname,void (*close_fn)(NBDClient *,bool ));
blockdev-nbd.c +1 -1
@@ -36,7 +36,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, gpointer opaque) {qio_channel_set_name (QIO_CHANNEL (cioc),"nbd-server" ); -nbd_client_new (NULL , cioc, +nbd_client_new (cioc, nbd_server->tlscreds,NULL , nbd_blockdev_client_closed); }
nbd/server.c +15 -38
88 lines not on this page · raw message
qemu-nbd.c +1 -1
@@ -353,7 +353,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, nb_fds++;nbd_update_server_watch (); -nbd_client_new (NULL , cioc, tlscreds,NULL , nbd_client_closed); +nbd_client_new (cioc, tlscreds,NULL , nbd_client_closed); }static void nbd_update_server_watch (void )
Signature · 1 line
2.18.0
Re: [Qemu-devel] [PATCH 2/2] nbd/server: drop old-style negotiation
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
After the previous commit,
nbd_client_newfirst parameter is always NULL. Let's drop it with all corresponding old-style negotiation code path which is unreachable now.
Being able to force oldstyle negotiation for interoperability testing may still be useful. But as fewer and fewer interesting clients exist that want oldstyle (after all, extensions are only usable with newstyle), I'm finding it hard to justify that we need qemu to be the oldstyle server for such interoperability testing (and I can always keep an older qemu binary around).
Quoted 12 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- include/block/nbd.h | 3 +-- blockdev-nbd.c | 2 +- nbd/server.c | 53 +++++++++++++-------------------------------- qemu-nbd.c | 2 +- 4 files changed, 18 insertions(+), 42 deletions(-)+++
b/blockdev-nbd.c
blockdev-nbd.c static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, · line 40
Quoted 6 lines
blockdev-nbd.c +1
-1@@ -36,7 +36,7 @@ static void nbd_accept(QIONetListener *listener, QIOChannelSocket *cioc, gpointer opaque) {qio_channel_set_name (QIO_CHANNEL (cioc),"nbd-server" ); - nbd_client_new(NULL, cioc, + nbd_client_new(cioc, nbd_server->tlscreds, NULL,
Could rewrap into fewer lines, but that's cosmetic.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 4 lines · Vladimir Sementsov-Ogievskiy
It's unexpected behavior that without -x option qemu-nbd do old-style negotiation. Let's use "" as a default name instead (as it is already done if tls is used) and therefore, drop old-style negotiation from Qemu NBD server.
Oddly enough, I wrote a similar patch in parallel, and am only now just seeing your mail. Yours is a bit stronger than mine (I added 'qemu-nbd -O' to allow explicit fallback to oldstyle, while you ripped it out altogether). The client can negotiate either style, so we don't need an option on the client side; rather, this is all about what the server should do by default.
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00568.html
Does anyone have a preference between the two? Here's the last time it was discussed:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03252.html
Quoted 11 lines
Vladimir Sementsov-Ogievskiy (2):
qemu-nbd: drop old-style negotiation
nbd/server: drop old-style negotiationinclude/block/nbd.h | 3 +-- blockdev-nbd.c | 2 +- nbd/server.c | 53 +++++++++++++-------------------------------- qemu-nbd.c | 25 +++++---------------- 4 files changed, 23 insertions(+), 60 deletions(-)
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
03.10.2018 20:32, Eric Blake wrote:
On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:
It's unexpected behavior that without -x option qemu-nbd do old-style negotiation. Let's use "" as a default name instead (as it is already done if tls is used) and therefore, drop old-style negotiation from Qemu NBD server.
Oddly enough, I wrote a similar patch in parallel, and am only now just seeing your mail. Yours is a bit stronger than mine (I added 'qemu-nbd -O' to allow explicit fallback to oldstyle, while you ripped it out altogether). The client can negotiate either style, so we don't need an option on the client side; rather, this is all about what the server should do by default.
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00568.html
Does anyone have a preference between the two? Here's the last time it was discussed:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03252.html
Hm, I think everyone who like new-style should answer that he don't care (except stricter way is a bit better: don't have options and code which we don't need). But if there is someone who has client which support only old-style negotiation his unhappiness (in case of strict way) will outweigh all our "bits":)
I'm from the first group). But let's chose your patch
Vladimir Sementsov-Ogievskiy (2):
qemu-nbd: drop old-style negotiation
nbd/server: drop old-style negotiation
include/block/nbd.h | 3 +-- blockdev-nbd.c | 2 +- nbd/server.c | 53 +++++++++++++-------------------------------- qemu-nbd.c | 25 +++++---------------- 4 files changed, 23 insertions(+), 60 deletions(-)
--
Best regards,
Vladimir
Quoted 6 lines · Vladimir Sementsov-Ogievskiy
03.10.2018 20:32, Eric Blake wrote:
On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:
It's unexpected behavior that without -x option qemu-nbd do old-style negotiation. Let's use "" as a default name instead (as it is already done if tls is used) and therefore, drop old-style negotiation from Qemu NBD server.
Hmm, your email quoting style changed from prior emails that used to prepend '>' when quoting, making it harder to tell where the text you are quoting ends,...
Quoted 9 lines
Oddly enough, I wrote a similar patch in parallel, and am only now just seeing your mail. Yours is a bit stronger than mine (I added 'qemu-nbd -O' to allow explicit fallback to oldstyle, while you ripped it out altogether). The client can negotiate either style, so we don't need an option on the client side; rather, this is all about what the server should do by default.
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00568.html
Does anyone have a preference between the two? Here's the last time it was discussed:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03252.html
and your reply begins.
Quoted 4 lines
Hm, I think everyone who like new-style should answer that he don't care (except stricter way is a bit better: don't have options and code which we don't need). But if there is someone who has client which support only old-style negotiation his unhappiness (in case of strict way) will outweigh all our "bits":)
I'm from the first group). But let's chose your patch
My argument in favor of your patch over mine: nbdkit is a GREAT testbed for forcing all sorts of integration testing scenarios, including oldstyle servers. Also, it includes a plugin for translating between new and oldstyle at will. That is, if we ever legitimately encounter a client that can only talk oldstyle, but qemu only talks newstyle, we just tell the user to connect:
old client => nbdkit -o nbd => newstyle qemu
and then qemu doesn't have to worry about oldstyle because nbdkit does instead.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
FWIW I don't have anything to add - agree with what's been said already.
Rich.
Signature · 5 lines
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Quoted 11 lines · Eric Blake
Quoted 6 lines · Vladimir Sementsov-Ogievskiy
03.10.2018 20:32, Eric Blake wrote:
On 10/3/18 12:02 PM, Vladimir Sementsov-Ogievskiy wrote:
It's unexpected behavior that without -x option qemu-nbd do old-style negotiation. Let's use "" as a default name instead (as it is already done if tls is used) and therefore, drop old-style negotiation from Qemu NBD server.Hmm, your email quoting style changed from prior emails that used to prepend '>' when quoting, making it harder to tell where the text you are quoting ends,...
I didn't change anything...
Quoted 35 lines
Quoted 9 lines
Oddly enough, I wrote a similar patch in parallel, and am only now just seeing your mail. Yours is a bit stronger than mine (I added 'qemu-nbd -O' to allow explicit fallback to oldstyle, while you ripped it out altogether). The client can negotiate either style, so we don't need an option on the client side; rather, this is all about what the server should do by default.
Quoted 7 lines
https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg00568.html
Does anyone have a preference between the two? Here's the last time it was discussed:
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03252.html
and your reply begins.
Quoted 8 lines
Hm, I think everyone who like new-style should answer that he don't care (except stricter way is a bit better: don't have options and code which we don't need).
But if there is someone who has client which support only old-style negotiation his unhappiness (in case of strict way) will outweigh all our "bits":)I'm from the first group). But let's chose your patch
My argument in favor of your patch over mine: nbdkit is a GREAT testbed for forcing all sorts of integration testing scenarios, including oldstyle servers. Also, it includes a plugin for translating between new and oldstyle at will. That is, if we ever legitimately encounter a client that can only talk oldstyle, but qemu only talks newstyle, we just tell the user to connect:
Quoted 5 lines
old client => nbdkit -o nbd => newstyle qemu
and then qemu doesn't have to worry about oldstyle because nbdkit does instead.
this is a bit more difficult and more overhead than option in Qemu.. But I don't sure we should care about
Signature · 2 lines
Best regards, Vladimir
Quoted 2 lines · Vladimir Sementsov-Ogievskiy
this is a bit more difficult and more overhead than option in Qemu.. But I don't sure we should care about
I think Eric's point is that dropping oldstyle in qemu-nbd isn't really a problem because nbdkit has no plans to drop oldstyle. So there will remain a path for very old clients to keep working (whether using nbdkit natively, or using nbdkit to proxy to qemu-nbd). Eric's example used the more complex proxy case. By contrast the nbdkit native case is very simple:
nbdkit -o file disk.img
(where the ‘-o’ flag switches to oldstyle protocol).
Rich.
Signature · 5 lines
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/
Quoted 9 lines · Vladimir Sementsov-Ogievskiy
It's unexpected behavior that without -x option qemu-nbd do old-style negotiation. Let's use "" as a default name instead (as it is already done if tls is used) and therefore, drop old-style negotiation from Qemu NBD server.
Vladimir Sementsov-Ogievskiy (2):
qemu-nbd: drop old-style negotiation
nbd/server: drop old-style negotiation
Quoted 6 lines
include/block/nbd.h | 3 +-- blockdev-nbd.c | 2 +- nbd/server.c | 53 +++++++++++++-------------------------------- qemu-nbd.c | 25 +++++---------------- 4 files changed, 23 insertions(+), 60 deletions(-)
Thanks; applying this series to my NBD queue.
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org