Libguestfs - Aug 2022 - [libnbd PATCH 1/2] api: Better use of nbd_internal_reset_size_and_flags

While trying to add a new API nbd_opt_set_meta_context(), I noticed
some existing oddities with nbd_opt_list_meta_context() and
nbd_opt_info().

Eric Blake (2):
  api: Better use of nbd_internal_reset_size_and_flags
  squash test

 generator/states-newstyle-opt-meta-context.c |  7 +++----
 lib/handle.c                                 |  4 ++++
 tests/opt-info.c                             | 20 +++++++++++++-------
 3 files changed, 20 insertions(+), 11 deletions(-)

-- 
2.37.2

The documentation for nbd_internal_reset_size_and_flags() claims we
should wipe all knowledge of previously-negotiated meta contexts when
swapping export names.  However, we weren't actually doing that, which
could lead to a user calling nbd_opt_info() for one export, then
switching to another export name, then having nbd_get_size() still
report the size of the old export.  At present, this is only mildly
confusing, but once the next patch exposes nbd_opt_set_meta_context()
for user control, it would become actively dangerous: negotiating meta
contexts for one export, then switching to a different export name
before nbd_opt_go, can lead us into calling nbd_cmd_block_status() when
the server is not expecting it.

While fixing this, code things to check whether we are actually
swapping export names; an idempotent nbd_set_export_name() is not
observable by the server, and therefore does not need to invalidate
what has already been negotiated.

Conversely, calling nbd_opt_list_meta_context() does not need to wipe
the list - it does not touch h->meta_contexts, but relies on the
user's callback instead.
---
 generator/states-newstyle-opt-meta-context.c | 7 +++----
 lib/handle.c                                 | 4 ++++
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/generator/states-newstyle-opt-meta-context.c
b/generator/states-newstyle-opt-meta-context.c
index 30b9617..76f1032 100644
--- a/generator/states-newstyle-opt-meta-context.c
+++ b/generator/states-newstyle-opt-meta-context.c
@@ -1,5 +1,5 @@
 /* nbd client library in userspace: state machine
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -28,7 +28,6 @@ STATE_MACHINE {
    * contexts, when doing SET (but an empty LIST is okay).
    */
   assert (h->gflags & LIBNBD_HANDSHAKE_FLAG_FIXED_NEWSTYLE);
-  nbd_internal_reset_size_and_flags (h);
   if (h->opt_current == NBD_OPT_LIST_META_CONTEXT) {
     assert (h->opt_mode);
     assert (h->structured_replies);
@@ -37,6 +36,8 @@ STATE_MACHINE {
   }
   else {
     assert (CALLBACK_IS_NULL (h->opt_cb.fn.context));
+    nbd_internal_reset_size_and_flags (h);
+    assert (h->meta_contexts == NULL);
     opt = NBD_OPT_SET_META_CONTEXT;
     if (!h->structured_replies || h->request_meta_contexts.len == 0) {
       SET_NEXT_STATE (%^OPT_GO.START);
@@ -44,8 +45,6 @@ STATE_MACHINE {
     }
   }

-  assert (h->meta_contexts == NULL);
-
   /* Calculate the length of the option request data. */
   len = 4 /* exportname len */ + strlen (h->export_name) + 4 /* nr queries
*/;
   for (i = 0; i < h->request_meta_contexts.len; ++i)
diff --git a/lib/handle.c b/lib/handle.c
index 8713e18..2aa89b9 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -219,6 +219,10 @@ nbd_unlocked_set_export_name (struct nbd_handle *h, const
char *export_name)
     return -1;
   }

+  if (strcmp (export_name, h->export_name) == 0)
+    return 0;
+
+  nbd_internal_reset_size_and_flags (h);
   new_name = strdup (export_name);
   if (!new_name) {
     set_error (errno, "strdup");
-- 
2.37.2

Add testsuite coverage that exposes the flaw fixed in the previous patch.

---
 tests/opt-info.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/tests/opt-info.c b/tests/opt-info.c
index b9739a5..2402a31 100644
--- a/tests/opt-info.c
+++ b/tests/opt-info.c
@@ -1,5 +1,5 @@
 /* NBD client library in userspace
- * Copyright (C) 2013-2020 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -80,15 +80,11 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  /* info on something not present fails, wipes out prior info */
+  /* changing export wipes out prior info */
   if (nbd_set_export_name (nbd, "a") == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
-  if (nbd_opt_info (nbd) != -1) {
-    fprintf (stderr, "expecting error for opt_info\n");
-    exit (EXIT_FAILURE);
-  }
   if (nbd_get_size (nbd) != -1) {
     fprintf (stderr, "expecting error for get_size\n");
     exit (EXIT_FAILURE);
@@ -102,7 +98,13 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  /* info for a different export */
+  /* info on something not present fails */
+  if (nbd_opt_info (nbd) != -1) {
+    fprintf (stderr, "expecting error for opt_info\n");
+    exit (EXIT_FAILURE);
+  }
+
+  /* info for a different export; idempotent name change is no-op */
   if (nbd_set_export_name (nbd, "b") == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
@@ -111,6 +113,10 @@ main (int argc, char *argv[])
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
+  if (nbd_set_export_name (nbd, "b") == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
   if ((r = nbd_get_size (nbd)) != 1) {
     fprintf (stderr, "expecting size of 1, got %" PRId64
"\n", r);
     exit (EXIT_FAILURE);
-- 
2.37.2