Libguestfs - Sep 2019 - [nbdkit PATCH 0/2] Make client fallback testing easier

This is similar to the recent --no-sr option - it's a change that is
unlikely to ever be used except by someone testing whether a client is
compliant to the protocol, but in that niche case, it can be quite
handy (it's a lot nicer to be able to purposefully cripple a server
from the command line than from a one-off compile, when testing if a
client's fallback for a spec-compliant but older server is correct).

I'm planning on writing a counterpart patch for libnbd to allow the
user to mask out bits that the client does not reply with (so you can
choose to cripple the protocol from either the server or the client
side, to see how the other side reacts, similar to the recently-added
nbd_set_request_structured_replies).

I'm open to naming suggestions on the command-line option.

Eric Blake (2):
  server: Skip option haggling from client lacking fixed newstyle
  server: Add --mask-handshake option for integration testing

 docs/nbdkit-protocol.pod             | 25 ++++++++++++++++++-----
 docs/synopsis.txt                    |  2 +-
 server/internal.h                    |  1 +
 server/options.h                     |  2 ++
 server/main.c                        | 30 ++++++++++++++++++----------
 server/protocol-handshake-newstyle.c | 14 +++++++++++--
 6 files changed, 55 insertions(+), 19 deletions(-)

-- 
2.21.0

The NBD protocol states that servers may still choose to honor various
NBD_OPT_* from a client that did not reply with
NBD_FLAG_C_FIXED_NEWSTYLE; however, for integration testing purposes,
it's a lot nicer if we reject everything except NBD_OPT_EXPORT_NAME
from such a client (for example, with this in place, we might have
spotted the bug fixed in commit e03b34d6 a bit sooner).  Thus, a
client that does not claim to understand fixed newstyle can now no
longer trigger TLS, structured replies, meta contexts, or the nicer
handling of NBD_OPT_GO.  All well-known clients listed in
nbdkit-protocol.pod default to requesting fixed newstyle, so this
shouldn't affect normal usage.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 server/protocol-handshake-newstyle.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 9ddc3198..486d416f 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -259,6 +259,16 @@ negotiate_handshake_newstyle_options (struct connection
*conn)
     option = be32toh (new_option.option);
     optname = name_of_nbd_opt (option);

+    /* If the client lacks fixed newstyle support, it should only send
+     * NBD_OPT_EXPORT_NAME.
+     */
+    if (!(conn->cflags & NBD_FLAG_FIXED_NEWSTYLE) &&
+        option != NBD_OPT_EXPORT_NAME) {
+      if (send_newstyle_option_reply (conn, option, NBD_REP_ERR_INVALID))
+        return -1;
+      continue;
+    }
+
     /* In --tls=require / FORCEDTLS mode the only options allowed
      * before TLS negotiation are NBD_OPT_ABORT and NBD_OPT_STARTTLS.
      */
-- 
2.21.0

Similar to --no-sr, it can be handy for testing a client
implementation to have a server that can easily be forced into older
behaviors, without having to recompile a one-off hack into a server or
dig up an older server binary that lacked a newer feature.

To see the patch in action, try things like:
$ ./nbdkit -U - -fv --mask-handshake=0 null \
  --run 'qemu-nbd --list -k $unixsocket'

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-protocol.pod             | 25 ++++++++++++++++++-----
 docs/synopsis.txt                    |  2 +-
 server/internal.h                    |  1 +
 server/options.h                     |  2 ++
 server/main.c                        | 30 ++++++++++++++++++----------
 server/protocol-handshake-newstyle.c |  4 ++--
 6 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod
index 3ae89063..272f4e5b 100644
--- a/docs/nbdkit-protocol.pod
+++ b/docs/nbdkit-protocol.pod
@@ -4,8 +4,8 @@ nbdkit - which parts of the NBD protocol nbdkit supports

 =head1 SYNOPSIS

- nbdkit [-n|--newstyle] [--no-sr] [-o|--oldstyle] [-e|--exportname EXPORTNAME]
-        [...]
+ nbdkit [-n|--newstyle] [--mask-handshake MASK] [--no-sr] [-o|--oldstyle]
+        [-e|--exportname EXPORTNAME] [...]

 =head1 DESCRIPTION

@@ -24,15 +24,30 @@ newstyle protocol is better in every respect than the
oldstyle
 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.
+block status.  By default, nbdkit advertises as many features as it
+can support (in some cases, this can be limited by what callbacks the
+plugin handles), even if the client does not negotiate to use all
+advertised features.

 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.
+Nbdkit also includes some options that are useful mainly when
+performing integration tests, for proving whether clients have sane
+fallback behavior when dealing various older servers permitted by the
+NBD protocol.  Use the I<--no-sr> flag to force the newstyle protocol
+to decline any client request for structured replies.  Use the
+I<--mask-handshake> parameter to mask off particular global features
+which are advertised during new-style handshake (defaulting to all
+supported bits set).  Clearing bit 0 (the low order bit) limits a
+client to using just C<NBD_OPT_EXPORT_NAME> (and is incompatible with
+TLS or structured replies); clearing bit 1 causes the handshake to
+send more padding bytes in response to C<NBD_OPT_EXPORT_NAME>.  Other
+bits in the mask will only have an effect if the NBD protocol is
+extended in the future to define other global bits.

 Use the I<-o> or I<--oldstyle> flag to force the oldstyle protocol.
+In this mode, I<--no-sr> and I<--mask-handshake> have no effect.

 =head2 Common clients and the protocol they require

diff --git a/docs/synopsis.txt b/docs/synopsis.txt
index 04cd136d..5fc57fd1 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] [--no-sr] [-o|--oldstyle]
+       [-n|--newstyle] [--mask-handshake MASK] [--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 9314e8ff..5da3e3c3 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -90,6 +90,7 @@ extern const char *exportname;
 extern bool foreground;
 extern const char *ipaddr;
 extern enum log_to log_to;
+extern unsigned mask_handshake;
 extern bool newstyle;
 extern bool no_sr;
 extern const char *port;
diff --git a/server/options.h b/server/options.h
index a69f413a..c74e0b8b 100644
--- a/server/options.h
+++ b/server/options.h
@@ -46,6 +46,7 @@ enum {
   FILTER_OPTION,
   LOG_OPTION,
   LONG_OPTIONS_OPTION,
+  MASK_HANDSHAKE_OPTION,
   NO_SR_OPTION,
   RUN_OPTION,
   SELINUX_LABEL_OPTION,
@@ -74,6 +75,7 @@ static const struct option long_options[] = {
   { "ipaddr",           required_argument, NULL, 'i' },
   { "log",              required_argument, NULL, LOG_OPTION },
   { "long-options",     no_argument,       NULL, LONG_OPTIONS_OPTION
},
+  { "mask-handshake",   required_argument, NULL,
MASK_HANDSHAKE_OPTION },
   { "new-style",        no_argument,       NULL, 'n' },
   { "newstyle",         no_argument,       NULL, 'n' },
   { "no-sr",            no_argument,       NULL, NO_SR_OPTION },
diff --git a/server/main.c b/server/main.c
index 22cf8d33..d433c1fa 100644
--- a/server/main.c
+++ b/server/main.c
@@ -67,6 +67,7 @@ const char *exportname;         /* -e */
 bool foreground;                /* -f */
 const char *ipaddr;             /* -i */
 enum log_to log_to = LOG_TO_DEFAULT; /* --log */
+unsigned mask_handshake = ~0U;  /* --mask-handshake */
 bool newstyle = true;           /* false = -o, true = -n */
 bool no_sr;                     /* --no-sr */
 char *pidfile;                  /* -P */
@@ -148,6 +149,7 @@ main (int argc, char *argv[])
   } *filter_filenames = NULL;
   size_t i;
   const char *magic_config_key;
+  char *end;

   /* Refuse to run if stdin/out/err are closed, whether or not -s is used. */
   if (fcntl (STDERR_FILENO, F_GETFL) == -1) {
@@ -348,6 +350,16 @@ main (int argc, char *argv[])
       ipaddr = optarg;
       break;

+    case MASK_HANDSHAKE_OPTION:
+      errno = 0;
+      mask_handshake = strtoul (optarg, &end, 0);
+      if (errno || *end) {
+        fprintf (stderr, "%s: cannot parse '%s' into
mask-handshake\n",
+                 program_name, optarg);
+        exit (EXIT_FAILURE);
+      }
+      break;
+
     case 'n':
       newstyle = true;
       break;
@@ -389,18 +401,14 @@ main (int argc, char *argv[])
       break;

     case 't':
-      {
-        char *end;
-
-        errno = 0;
-        threads = strtoul (optarg, &end, 0);
-        if (errno || *end) {
-          fprintf (stderr, "%s: cannot parse '%s' into
threads\n",
-                   program_name, optarg);
-          exit (EXIT_FAILURE);
-        }
-        /* XXX Worth a maximimum limit on threads? */
+      errno = 0;
+      threads = strtoul (optarg, &end, 0);
+      if (errno || *end) {
+        fprintf (stderr, "%s: cannot parse '%s' into
threads\n",
+                 program_name, optarg);
+        exit (EXIT_FAILURE);
       }
+      /* XXX Worth a maximimum limit on threads? */
       break;

     case 'U':
diff --git a/server/protocol-handshake-newstyle.c
b/server/protocol-handshake-newstyle.c
index 486d416f..9801f7c2 100644
--- a/server/protocol-handshake-newstyle.c
+++ b/server/protocol-handshake-newstyle.c
@@ -673,7 +673,7 @@ protocol_handshake_newstyle (struct connection *conn)
   struct new_handshake handshake;
   uint16_t gflags;

-  gflags = NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES;
+  gflags = (NBD_FLAG_FIXED_NEWSTYLE | NBD_FLAG_NO_ZEROES) & mask_handshake;

   debug ("newstyle negotiation: flags: global 0x%x", gflags);

@@ -694,7 +694,7 @@ protocol_handshake_newstyle (struct connection *conn)
   /* ... which we check for accuracy. */
   debug ("newstyle negotiation: client flags: 0x%x",
conn->cflags);
   if (conn->cflags & ~gflags) {
-    nbdkit_error ("client requested unknown flags 0x%x",
conn->cflags);
+    nbdkit_error ("client requested unexpected flags 0x%x",
conn->cflags);
     return -1;
   }

-- 
2.21.0

ACK series.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org