Eric Blake
2019-Aug-20 19:28 UTC
[Libguestfs] [nbdkit PATCH v2] main: Add option to disable SR advertisement
When we added support for .extents, we had nbdkit unconditionally
support structured replies if the client requests them, and the
plugin's .can_extents has no impact on what the server advertises.
However, while the plugin API doesn't care whether the client
requested SR, there are still integration situations where not
advertising SR can be useful (such as comparison on what a client does
with no block status vs. a block status that always reports
allocated). We already have the command line options -o/-n for
tweaking core server functionality; add --no-sr to the mix.
In particular, doing this found that 'qemu-nbd --list' from qemu 4.2
is rather picky: it hangs up on a server that replies with
NBD_REP_ERR_POLICY, rather than silently proceeding without SR support
(at least libnbd is more tolerant).
Signed-off-by: Eric Blake <eblake@redhat.com>
---
Looks much different as a command line option instead of a hack to
the noextents filter, but I like the result a lot better.
I'm open to bike-shed opinions on the option name, or even if we want
to emulate a tri-state option --protocol=[old|no-sr|new] (with -o and
-n remaining synonyms for 2 of the 3 options for back-compat). I
purposefully did not burn a short-option letter on the new option, as
it is unlikely to be commonly needed.
docs/nbdkit-plugin.pod | 5 +++-
docs/nbdkit-protocol.pod | 21 ++++++++++++----
docs/nbdkit.pod | 12 ++++++++--
docs/synopsis.txt | 2 +-
server/internal.h | 1 +
server/options.h | 4 +++-
server/main.c | 5 ++++
server/protocol-handshake-newstyle.c | 14 +++++++++++
tests/test-eflags.sh | 36 ++++++++++++++++++++++++++++
9 files changed, 91 insertions(+), 9 deletions(-)
diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index 423cccdb..bc3d9749 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -833,7 +833,10 @@ allocated, sparse and zeroed regions of the disk.
This function will not be called if C<.can_extents> returned false.
nbdkit's default behaviour in this case is to treat the whole virtual
-disk as if it was allocated.
+disk as if it was allocated. Also, this function will not be called
+by a client that does not request structured replies (the I<--no-sr>
+option of nbdkit can be used to test behavior when C<.extents> is
+unavailable to the client).
The callback should detect and return the list of extents overlapping
the range C<[offset...offset+count-1]>. The C<extents> parameter
diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod
index ad470bd4..35db07b3 100644
--- a/docs/nbdkit-protocol.pod
+++ b/docs/nbdkit-protocol.pod
@@ -4,7 +4,7 @@ nbdkit - which parts of the NBD protocol nbdkit supports
=head1 SYNOPSIS
- nbdkit [-n|--newstyle] [-o|--oldstyle] [-e|--exportname EXPORTNAME]
+ nbdkit [-n|--newstyle] [--no-sr] [-o|--oldstyle] [-e|--exportname EXPORTNAME]
[...]
=head1 DESCRIPTION
@@ -21,11 +21,17 @@ be negotiated from the server side.
nbdkit defaults to the newstyle protocol since nbdkit E<ge> 1.3. The
newstyle protocol is better in every respect than the oldstyle
-protocol and you should prefer it if possible.
+protocol and you should prefer it if possible. The newstyle protocol
+also includes an extension where a client may request structured
+replies for even more capabilities, such as sparse reads or obtaining
+block status.
Use the I<-e> or I<--exportname> flag to set the optional
exportname
for the newstyle protocol.
+Use the I<--no-sr> flag to force the newstyle protocol to decline any
+client request for structured replies.
+
Use the I<-o> or I<--oldstyle> flag to force the oldstyle protocol.
=head2 Common clients and the protocol they require
@@ -35,10 +41,13 @@ Use the I<-o> or I<--oldstyle> flag to force the
oldstyle protocol.
qemu <= 2.5 without exportname oldstyle
qemu <= 2.5 with exportname newstyle
qemu >= 2.6 client can talk either protocol
+ qemu >= 2.11 client tries structured replies
nbd-client < 3.10 client can talk either protocol
- nbd-client >= 3.10 newstyle
+ nbd-client >= 3.10 newstyle, no structured replies
any TLS (encrypted) client newstyle
nbdkit nbd plugin client can talk either protocol
+ nbdkit >= 1.13.3 nbd plugin tries structured replies
+ libnbd either protocol, tries structured replies
=head2 Errors seen if using the wrong protocol
@@ -128,6 +137,10 @@ However we don’t expose the capability to send structured
replies to
plugins yet, nor do we send human-readable error messages using this
facility.
+In nbdkit E<ge> 1.13.9>, the command-line option I<--no-rc> can
be
+used to disable server support for structured replies, for testing
+client fallbacks.
+
=item Metadata Querying
Supported in nbdkit E<ge> 1.11.8.
@@ -182,4 +195,4 @@ Pino Toscano
=head1 COPYRIGHT
-Copyright (C) 2013-2018 Red Hat Inc.
+Copyright (C) 2013-2019 Red Hat Inc.
diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod
index 21a5207f..367a164d 100644
--- a/docs/nbdkit.pod
+++ b/docs/nbdkit.pod
@@ -264,10 +264,18 @@ For more details see L<nbdkit-service(1)/LOGGING>.
=item B<--newstyle>
-Use the newstyle NBD protocol protocol. This is the default in nbdkit
+Use the newstyle NBD protocol. This is the default in nbdkit
E<ge> 1.3. In earlier versions the default was oldstyle.
See L<nbdkit-protocol(1)>.
+=item B<--no-sr>
+
+Do not advertise structured replies. A client must request structured
+replies to take advantage of block status and potential sparse reads;
+however, as structured reads are not a mandatory part of the newstyle
+NBD protocol, this option can be used to debug client fallbacks for
+dealing with older servers. See L<nbdkit-protocol(1)>.
+
=item B<-o>
=item B<--old-style>
@@ -622,4 +630,4 @@ Pino Toscano
=head1 COPYRIGHT
-Copyright (C) 2013-2018 Red Hat Inc.
+Copyright (C) 2013-2019 Red Hat Inc.
diff --git a/docs/synopsis.txt b/docs/synopsis.txt
index 1a9d33d8..04cd136d 100644
--- a/docs/synopsis.txt
+++ b/docs/synopsis.txt
@@ -3,7 +3,7 @@ nbdkit [-D|--debug PLUGIN|FILTER.FLAG=N]
[--filter FILTER ...] [-f|--foreground]
[-g|--group GROUP] [-i|--ipaddr IPADDR]
[--log stderr|syslog]
- [-n|--newstyle] [-o|--oldstyle]
+ [-n|--newstyle] [--no-sr] [-o|--oldstyle]
[-P|--pidfile PIDFILE]
[-p|--port PORT] [-r|--readonly]
[--run CMD] [-s|--single] [--selinux-label LABEL]
diff --git a/server/internal.h b/server/internal.h
index 29a89606..22e13b6d 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -88,6 +88,7 @@ extern bool foreground;
extern const char *ipaddr;
extern enum log_to log_to;
extern bool newstyle;
+extern bool no_sr;
extern const char *port;
extern bool readonly;
extern const char *run;
diff --git a/server/options.h b/server/options.h
index 0be19f15..a69f413a 100644
--- a/server/options.h
+++ b/server/options.h
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2018 Red Hat Inc.
+ * Copyright (C) 2013-2019 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -46,6 +46,7 @@ enum {
FILTER_OPTION,
LOG_OPTION,
LONG_OPTIONS_OPTION,
+ NO_SR_OPTION,
RUN_OPTION,
SELINUX_LABEL_OPTION,
SHORT_OPTIONS_OPTION,
@@ -75,6 +76,7 @@ static const struct option long_options[] = {
{ "long-options", no_argument, NULL, LONG_OPTIONS_OPTION
},
{ "new-style", no_argument, NULL, 'n' },
{ "newstyle", no_argument, NULL, 'n' },
+ { "no-sr", no_argument, NULL, NO_SR_OPTION },
{ "old-style", no_argument, NULL, 'o' },
{ "oldstyle", no_argument, NULL, 'o' },
{ "pid-file", required_argument, NULL, 'P' },
diff --git a/server/main.c b/server/main.c
index 963a4871..65025a62 100644
--- a/server/main.c
+++ b/server/main.c
@@ -68,6 +68,7 @@ bool foreground; /* -f */
const char *ipaddr; /* -i */
enum log_to log_to = LOG_TO_DEFAULT; /* --log */
bool newstyle = true; /* false = -o, true = -n */
+bool no_sr; /* --no-sr */
char *pidfile; /* -P */
const char *port; /* -p */
bool readonly; /* -r */
@@ -341,6 +342,10 @@ main (int argc, char *argv[])
newstyle = true;
break;
+ case NO_SR_OPTION:
+ no_sr = true;
+ break;
+
case 'o':
newstyle = false;
break;
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index e0136de1..6ccb76f3 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -481,6 +481,17 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
debug ("newstyle negotiation: %s: client requested structured
replies",
name_of_nbd_opt (option));
+ if (no_sr) {
+ /* Must fail with ERR_UNSUP for qemu 4.2 to remain happy;
+ * but failing with ERR_POLICY would have been nicer.
+ */
+ if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_UNSUP) == -1)
+ return -1;
+ debug ("newstyle negotiation: %s: structured replies are
disabled",
+ name_of_nbd_opt (option));
+ break;
+ }
+
if (send_newstyle_option_reply (conn, option, NBD_REP_ACK) == -1)
return -1;
@@ -499,6 +510,9 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
if (conn_recv_full (conn, data, optlen, "read: %s: %m",
optname) == -1)
return -1;
+ /* Note that we support base:allocation whether or not the plugin
+ * supports can_extents.
+ */
if (!conn->structured_replies) {
if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID)
== -1)
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
index 6cc61631..f5cd43ed 100755
--- a/tests/test-eflags.sh
+++ b/tests/test-eflags.sh
@@ -87,6 +87,8 @@ fail ()
#----------------------------------------------------------------------
# can_write=false
+#
+# nbdkit supports DF if client requests SR.
do_nbdkit <<'EOF'
case "$1" in
@@ -98,6 +100,22 @@ EOF
[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
fail "$LINENO: expected HAS_FLAGS|READ_ONLY|SEND_DF"
+#----------------------------------------------------------------------
+# --no-sr
+# can_write=false
+#
+# When SR is disabled, so is the DF flag.
+
+do_nbdkit --no-sr <<'EOF'
+case "$1" in
+ get_size) echo 1M ;;
+ *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY )) ] ||
+ fail "$LINENO: expected HAS_FLAGS|READ_ONLY"
+
#----------------------------------------------------------------------
# -r
# can_write=false
@@ -147,6 +165,24 @@ EOF
[ $eflags -eq $(( HAS_FLAGS|SEND_DF )) ] ||
fail "$LINENO: expected HAS_FLAGS|SEND_DF"
+#----------------------------------------------------------------------
+# --no=sr
+# --filter=nozero
+# can_write=true
+#
+# Absolute minimum in flags.
+
+do_nbdkit --no-sr --filter=nozero <<'EOF'
+case "$1" in
+ get_size) echo 1M ;;
+ can_write) exit 0 ;;
+ *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS )) ] ||
+ fail "$LINENO: expected HAS_FLAGS"
+
#----------------------------------------------------------------------
# -r
# can_write=true
--
2.21.0
Eric Blake
2019-Aug-20 19:32 UTC
Re: [Libguestfs] [nbdkit PATCH v2] main: Add option to disable SR advertisement
On 8/20/19 2:28 PM, Eric Blake wrote:> When we added support for .extents, we had nbdkit unconditionally > support structured replies if the client requests them, and the > plugin's .can_extents has no impact on what the server advertises. > However, while the plugin API doesn't care whether the client > requested SR, there are still integration situations where not > advertising SR can be useful (such as comparison on what a client does > with no block status vs. a block status that always reports > allocated). We already have the command line options -o/-n for > tweaking core server functionality; add --no-sr to the mix. > > In particular, doing this found that 'qemu-nbd --list' from qemu 4.2 > is rather picky: it hangs up on a server that replies with > NBD_REP_ERR_POLICY, rather than silently proceeding without SR support > (at least libnbd is more tolerant). > > Signed-off-by: Eric Blake <eblake@redhat.com> > --- > > Looks much different as a command line option instead of a hack to > the noextents filter, but I like the result a lot better. > > I'm open to bike-shed opinions on the option name, or even if we want > to emulate a tri-state option --protocol=[old|no-sr|new] (with -o and > -n remaining synonyms for 2 of the 3 options for back-compat). I > purposefully did not burn a short-option letter on the new option, as > it is unlikely to be commonly needed. > > docs/nbdkit-plugin.pod | 5 +++- > docs/nbdkit-protocol.pod | 21 ++++++++++++---- > docs/nbdkit.pod | 12 ++++++++-- > docs/synopsis.txt | 2 +- > server/internal.h | 1 + > server/options.h | 4 +++- > server/main.c | 5 ++++ > server/protocol-handshake-newstyle.c | 14 +++++++++++ > tests/test-eflags.sh | 36 ++++++++++++++++++++++++++++ > 9 files changed, 91 insertions(+), 9 deletions(-)Oh, and I missed squashing this in: diff --git i/filters/noextents/nbdkit-noextents-filter.pod w/filters/noextents/nbdkit-noextents-filter.pod index 47223928..9cc586ff 100644 --- i/filters/noextents/nbdkit-noextents-filter.pod +++ w/filters/noextents/nbdkit-noextents-filter.pod @@ -11,7 +11,11 @@ nbdkit-noextents-filter - disable extents in the underlying plugin “Extents” are a feature of the NBD protocol / nbdkit which allow the client to detect sparse regions of the underlying disk. C<nbdkit-noextents-filter> disables this so that the plugin appears to -be fully allocated. +be fully allocated, at least to a client that requests structured +replies. It is also possible to use the I<--no-sr> option to nbdkit +to prevent the client from using structured replies, which among its +other side effects will prevent the client from querying extents at +all. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2019-Aug-20 21:03 UTC
Re: [Libguestfs] [nbdkit PATCH v2] main: Add option to disable SR advertisement
On Tue, Aug 20, 2019 at 02:32:08PM -0500, Eric Blake wrote:> On 8/20/19 2:28 PM, Eric Blake wrote: > > When we added support for .extents, we had nbdkit unconditionally > > support structured replies if the client requests them, and the > > plugin's .can_extents has no impact on what the server advertises. > > However, while the plugin API doesn't care whether the client > > requested SR, there are still integration situations where not > > advertising SR can be useful (such as comparison on what a client does > > with no block status vs. a block status that always reports > > allocated). We already have the command line options -o/-n for > > tweaking core server functionality; add --no-sr to the mix. > > > > In particular, doing this found that 'qemu-nbd --list' from qemu 4.2 > > is rather picky: it hangs up on a server that replies with > > NBD_REP_ERR_POLICY, rather than silently proceeding without SR support > > (at least libnbd is more tolerant). > > > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > > > > Looks much different as a command line option instead of a hack to > > the noextents filter, but I like the result a lot better. > > > > I'm open to bike-shed opinions on the option name, or even if we want > > to emulate a tri-state option --protocol=[old|no-sr|new] (with -o and > > -n remaining synonyms for 2 of the 3 options for back-compat). I > > purposefully did not burn a short-option letter on the new option, as > > it is unlikely to be commonly needed. > > > > docs/nbdkit-plugin.pod | 5 +++- > > docs/nbdkit-protocol.pod | 21 ++++++++++++---- > > docs/nbdkit.pod | 12 ++++++++-- > > docs/synopsis.txt | 2 +- > > server/internal.h | 1 + > > server/options.h | 4 +++- > > server/main.c | 5 ++++ > > server/protocol-handshake-newstyle.c | 14 +++++++++++ > > tests/test-eflags.sh | 36 ++++++++++++++++++++++++++++ > > 9 files changed, 91 insertions(+), 9 deletions(-) > > Oh, and I missed squashing this in: > > diff --git i/filters/noextents/nbdkit-noextents-filter.pod > w/filters/noextents/nbdkit-noextents-filter.pod > index 47223928..9cc586ff 100644 > --- i/filters/noextents/nbdkit-noextents-filter.pod > +++ w/filters/noextents/nbdkit-noextents-filter.pod > @@ -11,7 +11,11 @@ nbdkit-noextents-filter - disable extents in the > underlying plugin > “Extents” are a feature of the NBD protocol / nbdkit which allow the > client to detect sparse regions of the underlying disk. > C<nbdkit-noextents-filter> disables this so that the plugin appears to > -be fully allocated. > +be fully allocated, at least to a client that requests structured > +replies. It is also possible to use the I<--no-sr> option to nbdkit > +to prevent the client from using structured replies, which among its > +other side effects will prevent the client from querying extents at > +all.Yes, the two patches squashed look good, ACK. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Possibly Parallel Threads
- [nbdkit PATCH 2/2] server: Add --mask-handshake option for integration testing
- [nbdkit PATCH] noextents: Add hook to cripple SR advertisement
- [nbdkit PATCH 0/2] Make client fallback testing easier
- [PATCH nbdkit] server: Deprecate the -e/--exportname parameter.
- [PATCH nbdkit] server: Use bool for types which are really booleans.