Libguestfs - Sep 2019 - Re: [libnbd PATCH] api: Add way to avoid structured replies

We want to default to requesting structured replies, whether or not
that request will be honored (it's essential for efficient sparse file
reads and the DF flag for structured pread, as well as for meta
context support even if we do not request a default meta context).
However, for integration testing, it can be nice to easily request a
client that does not request structured replies.

We can test this by reusing our eflags test.  Note that nbdkit does
not provide a 'can_df' callback in the sh script (so our key=value
override is silently ignored), rather, we control whether nbdkit
advertises df based on whether we request structured replies.
---

I'm open to renaming the API to the shorter 'nbd_set_request_sr' if
the existing name choice seems too long.

This is a counterpart to the recent addition of --no-sr in nbdkit 1.14
for server-side disable; but by doing it in the client side, we can
test lack of structured replies even while still using nbdkit 1.12.x.

 lib/internal.h                                |  1 +
 generator/generator                           | 30 +++++++++++++++++++
 .../states-newstyle-opt-structured-reply.c    |  5 ++++
 lib/handle.c                                  | 15 ++++++++++
 tests/Makefile.am                             | 18 +++++++++++
 tests/eflags.c                                |  6 ++++
 TODO                                          |  2 +-
 7 files changed, 76 insertions(+), 1 deletion(-)

diff --git a/lib/internal.h b/lib/internal.h
index 581777a..a48edff 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -73,6 +73,7 @@ struct nbd_handle {
   char *tls_psk_file;           /* PSK filename, NULL = no PSK */

   /* Desired metadata contexts. */
+  bool request_sr;
   char **request_meta_contexts;

   /* Global flags from the server. */
diff --git a/generator/generator b/generator/generator
index 1cc5c19..5c32afa 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1243,6 +1243,36 @@ Get the current TLS PSK filename.";
   };
 *)

+  "set_request_structured_replies", {
+    default_call with
+    args = [Bool "request"]; ret = RErr;
+    permitted_states = [ Created ];
+    first_version = (1, 2);
+    shortdesc = "control use of structured replies";
+    longdesc = "\
+By default, libnbd tries to negotiate structured replies with the
+server, as this protocol extension must be in use before
+C<nbd_can_meta_context> or C<nbd_can_df> can return true.  However,
+for integration testing, it can be useful to clear this flag
+rather than find a way to alter the server to fail the negotiation
+request.";
+    see_also = ["L<nbd_get_request_structured_replies(3)>";
+                "L<nbd_can_meta_context(3)>";
"L<nbd_can_df(3)>"];
+  };
+
+  "get_request_structured_replies", {
+    default_call with
+    args = []; ret = RBool;
+    first_version = (1, 2);
+    shortdesc = "see if structured replies are attempted";
+    longdesc = "\
+Return the state of the request structured replies flag on this
+handle.  Note that this only reports whether the client attempts
+to negotiate structured replies, and not whether the server was
+able to honor that request";
+    see_also = ["L<nbd_set_request_structured_replies(3)>"];
+  };
+
   "add_meta_context", {
     default_call with
     args = [ String "name" ]; ret = RErr;
diff --git a/generator/states-newstyle-opt-structured-reply.c
b/generator/states-newstyle-opt-structured-reply.c
index d932248..415f7e0 100644
--- a/generator/states-newstyle-opt-structured-reply.c
+++ b/generator/states-newstyle-opt-structured-reply.c
@@ -20,6 +20,11 @@

 /* STATE MACHINE */ {
  NEWSTYLE.OPT_STRUCTURED_REPLY.START:
+  if (!h->request_sr) {
+    SET_NEXT_STATE (%^OPT_SET_META_CONTEXT.START);
+    return 0;
+  }
+
   h->sbuf.option.version = htobe64 (NBD_NEW_VERSION);
   h->sbuf.option.option = htobe32 (NBD_OPT_STRUCTURED_REPLY);
   h->sbuf.option.optlen = htobe32 (0);
diff --git a/lib/handle.c b/lib/handle.c
index f8cc83a..c23ef01 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -63,6 +63,7 @@ nbd_create (void)

   h->unique = 1;
   h->tls_verify_peer = true;
+  h->request_sr = true;

   s = getenv ("LIBNBD_DEBUG");
   h->debug = s && strcmp (s, "1") == 0;
@@ -242,6 +243,20 @@ nbd_unlocked_add_meta_context (struct nbd_handle *h, const
char *name)
   return 0;
 }

+int
+nbd_unlocked_set_request_structured_replies (struct nbd_handle *h,
+                                             bool request)
+{
+  h->request_sr = request;
+  return 0;
+}
+
+int
+nbd_unlocked_get_request_structured_replies (struct nbd_handle *h)
+{
+  return h->request_sr;
+}
+
 const char *
 nbd_unlocked_get_package_name (struct nbd_handle *h)
 {
diff --git a/tests/Makefile.am b/tests/Makefile.am
index a7bc1b5..41b620d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -147,6 +147,8 @@ check_PROGRAMS += \
 	can-not-zero-flag \
 	can-fast-zero-flag \
 	can-not-fast-zero-flag \
+	can-df-flag \
+	can-not-df-flag \
 	can-multi-conn-flag \
 	can-not-multi-conn-flag \
 	can-cache-flag \
@@ -181,6 +183,8 @@ TESTS += \
 	can-not-zero-flag \
 	can-fast-zero-flag \
 	can-not-fast-zero-flag \
+	can-df-flag \
+	can-not-df-flag \
 	can-multi-conn-flag \
 	can-not-multi-conn-flag \
 	can-cache-flag \
@@ -309,6 +313,20 @@ can_not_fast_zero_flag_CPPFLAGS = \
 can_not_fast_zero_flag_CFLAGS = $(WARNINGS_CFLAGS)
 can_not_fast_zero_flag_LDADD = $(top_builddir)/lib/libnbd.la

+can_df_flag_SOURCES = eflags.c
+can_df_flag_CPPFLAGS = \
+	-I$(top_srcdir)/include -Dflag=can_df \
+	$(NULL)
+can_df_flag_CFLAGS = $(WARNINGS_CFLAGS)
+can_df_flag_LDADD = $(top_builddir)/lib/libnbd.la
+
+can_not_df_flag_SOURCES = eflags.c
+can_not_df_flag_CPPFLAGS = \
+	-I$(top_srcdir)/include -Dflag=can_df -Dvalue=false -Dno_sr \
+	$(NULL)
+can_not_df_flag_CFLAGS = $(WARNINGS_CFLAGS)
+can_not_df_flag_LDADD = $(top_builddir)/lib/libnbd.la
+
 can_multi_conn_flag_SOURCES = eflags.c
 can_multi_conn_flag_CPPFLAGS = \
 	-I$(top_srcdir)/include -Dflag=can_multi_conn \
diff --git a/tests/eflags.c b/tests/eflags.c
index 675802a..afff3a9 100644
--- a/tests/eflags.c
+++ b/tests/eflags.c
@@ -84,6 +84,12 @@ main (int argc, char *argv[])
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
+#ifdef no_sr
+  if (nbd_set_request_structured_replies (nbd, false) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+#endif
   if (nbd_connect_command (nbd, args) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
diff --git a/TODO b/TODO
index 34bb983..642d39f 100644
--- a/TODO
+++ b/TODO
@@ -48,7 +48,7 @@ Suggested API improvements:
   - support PKCS11 URIs (RFC 7512)

   Easier server implementation testing:
-  - a way to disable requesting structured replies
+  - a way to force NBD_OPT_EXPORT_NAME over NBD_OPT_GO
   - a way to forcefully violate protocol (such as allowing writes to a
     readonly connection, or sending a request that exceeds bounds) for
     testing server reactions
-- 
2.21.0

On Wed, Sep 04, 2019 at 01:28:08PM -0500, Eric Blake
wrote:
> We want to default to requesting structured replies, whether or not
> that request will be honored (it's essential for efficient sparse file
> reads and the DF flag for structured pread, as well as for meta
> context support even if we do not request a default meta context).
> However, for integration testing, it can be nice to easily request a
> client that does not request structured replies.
> 
> We can test this by reusing our eflags test.  Note that nbdkit does
> not provide a 'can_df' callback in the sh script (so our key=value
> override is silently ignored), rather, we control whether nbdkit
> advertises df based on whether we request structured replies.
> ---
> 
> I'm open to renaming the API to the shorter
'nbd_set_request_sr' if
> the existing name choice seems too long.
There's not really a limit on the length of API names, and in this
case the longer name explains what the option does more clearly.

Anyway this looks fine to me.

ACK

I have one comment unrelated to the patch:
> +  "set_request_structured_replies", {
> +    default_call with
> +    args = [Bool "request"]; ret = RErr;
> +    permitted_states = [ Created ];
> +    first_version = (1, 2);
I just know that we're going to end up adding new APIs and forgetting
to set the first_version field.  There are various things we could do
to prevent this:

(1) In ‘default_call’ set first_version = (0, 0).  Update all existing
calls with first_version = (1, 0).  Then add a check that
first_version <> (0, 0).  There's still a danger of copy and paste
from an existing API, but we should be able to catch that in review.

(2) Store first_version in a separate table.  Add checks to ensure the
new table exhaustively covers all APIs.  It should be obvious when
submitting a new API that the first_version table must be updated and
what to add here:

  let first_version = [
    "set_debug", (1, 0);
    ...
    "set_request_structured_replies", (1, 2);
    "get_request_structured_replies", (1, 2);
  ]

Not sure which is better.

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

On 9/5/19 2:40 AM, Richard W.M. Jones wrote:
> I have one comment unrelated to the patch:
> 
>> +  "set_request_structured_replies", {
>> +    default_call with
>> +    args = [Bool "request"]; ret = RErr;
>> +    permitted_states = [ Created ];
>> +    first_version = (1, 2);
> 
> I just know that we're going to end up adding new APIs and forgetting
> to set the first_version field.  There are various things we could do
> to prevent this:
> 
> (1) In ‘default_call’ set first_version = (0, 0).  Update all existing
> calls with first_version = (1, 0).  Then add a check that
> first_version <> (0, 0).  There's still a danger of copy and
paste
> from an existing API, but we should be able to catch that in review.
Or maybe couple that with a declaration that maps version (1, 0) has X
signatures and version (1, 2) has Y releases, then if we detect a count
mismatch, it must be a new addition incorrectly versioned.
> 
> (2) Store first_version in a separate table.  Add checks to ensure the
> new table exhaustively covers all APIs.  It should be obvious when
> submitting a new API that the first_version table must be updated and
> what to add here:
> 
>   let first_version = [
>     "set_debug", (1, 0);
>     ...
>     "set_request_structured_replies", (1, 2);
>     "get_request_structured_replies", (1, 2);
>   ]
This approach also seems fine (it's a bit closer to maintaining the
.syms file by hand, but with the compiler guaranteeing that we touch
.syms for everything we add).  I lean slightly towards option 2.

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