Libguestfs - Jun 2019 - [libnbd PATCH] block-status: Make callback usage consistent with pread_structured

Now that we have a way to pass Mutable(Int "error") to callbacks,
that's a much nicer way for language bindings to use than relying on
the value of errno after returning -1. Update the semantics for
block status to match what pread_structured does, getting errno out
of the equation, and changing things to call the callback for a
second context even after an earlier callback failure sets the error.

This is an API/ABI break to existing clients, but that's okay because
we have not yet declared stable API.  (If you need to compile against
libnbd 0.4 and 0.5 simultaneously, you can use
#ifdef LIBNBD_HAVE_NBD_PREAD_STRUCTURED
as a witness for the new signature rather than the old).

---

This applies on top of my changes to let pread_structured use Mutable.
I'll go ahead and push it, so we can cut a release soon.

 generator/generator                 | 17 +++++----
 generator/states-reply-structured.c | 56 ++++++++++++++---------------
 interop/dirty-bitmap.c              | 15 ++++----
 lib/internal.h                      |  2 +-
 ocaml/examples/extents.ml           |  3 +-
 python/t/460-block-status.py        |  3 +-
 tests/meta-base-allocation.c        |  9 ++---
 7 files changed, 54 insertions(+), 51 deletions(-)

diff --git a/generator/generator b/generator/generator
index ec09836..c29460c 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1527,7 +1527,8 @@ punching a hole.";
              Callback ("extent", [Opaque "data"; String
"metacontext";
                                   UInt64 "offset";
                                   ArrayAndLen (UInt32 "entries",
-                                               "nr_entries")]);
+                                               "nr_entries");
+                                  Mutable (Int "error")]);
              Flags "flags" ];
     ret = RErr;
     permitted_states = [ Connected ];
@@ -1548,10 +1549,11 @@ supported by the server (see
C<nbd_can_meta_context>) this call
 returns information about extents by calling back to the
 C<extent> function.  The callback cannot call C<nbd_*> APIs on the
 same handle since it holds the handle lock and will
-cause a deadlock.  If the callback returns C<-1>, any remaining
-contexts will be ignored and the overall block status command
-will fail with the same value of C<errno> left after the
-failing callback.
+cause a deadlock.  If the callback returns C<-1>, and no earlier
+error has been detected, then the overall block status command
+will fail with any non-zero value stored into the callback's
+C<error> parameter (with a default of C<EPROTO>); but any further
+contexts will still invoke the callback.

 The C<extent> function is called once per type of metadata available,
 with the C<data> passed to this function.  The C<metacontext>
@@ -1564,6 +1566,8 @@ NBD protocol document in the section about
 C<NBD_REPLY_TYPE_BLOCK_STATUS> describes the meaning of this array;
 for contexts known to libnbd, B<E<lt>libnbd.hE<gt>> contains
constants
 beginning with C<LIBNBD_STATE_> that may help decipher the values.
+On entry to the callback, the C<error> parameter contains the errno
+value of any previously detected error.

 It is possible for the extent function to be called
 more times than you expect (if the server is buggy),
@@ -1812,7 +1816,8 @@ in C<nbd_zero>.";
              CallbackPersist ("extent", [Opaque "data";
String "metacontext";
                                          UInt64 "offset";
                                          ArrayAndLen (UInt32
"entries",
-                                                     
"nr_entries")]);
+                                                      "nr_entries");
+                                         Mutable (Int "error")]);
              Flags "flags" ];
     ret = RInt64;
     permitted_states = [ Connected ];
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index fa11dd6..91c6215 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -478,37 +478,33 @@
     assert (h->bs_entries);
     assert (length >= 12);

-    if (cmd->error)
+    /* Need to byte-swap the entries returned, but apart from that we
+     * don't validate them.
+     */
+    for (i = 0; i < length/4; ++i)
+      h->bs_entries[i] = be32toh (h->bs_entries[i]);
+
+    /* Look up the context ID. */
+    context_id = h->bs_entries[0];
+    for (meta_context = h->meta_contexts;
+         meta_context;
+         meta_context = meta_context->next)
+      if (context_id == meta_context->context_id)
+        break;
+
+    if (meta_context) {
+      /* Call the caller's extent function. */
+      int error = cmd->error;
+
+      if (cmd->cb.fn.extent (cmd->cb.opaque, meta_context->name,
cmd->offset,
+                             &h->bs_entries[1], (length-4) / 4,
&error) == -1)
+        if (cmd->error == 0)
+          cmd->error = error ? error : EPROTO;
+    }
+    else
       /* Emit a debug message, but ignore it. */
-      debug (h, "ignoring meta context ID %" PRIu32 " after
callback failure",
-             be32toh (h->bs_entries[0]));
-    else {
-      /* Need to byte-swap the entries returned, but apart from that we
-       * don't validate them.
-       */
-      for (i = 0; i < length/4; ++i)
-        h->bs_entries[i] = be32toh (h->bs_entries[i]);
-
-      /* Look up the context ID. */
-      context_id = h->bs_entries[0];
-      for (meta_context = h->meta_contexts;
-           meta_context;
-           meta_context = meta_context->next)
-        if (context_id == meta_context->context_id)
-          break;
-
-      if (meta_context) {
-        /* Call the caller's extent function. */
-        errno = 0;
-        if (cmd->cb.fn.extent (cmd->cb.opaque, meta_context->name,
cmd->offset,
-                               &h->bs_entries[1], (length-4) / 4) == -1)
-          cmd->error = errno ? errno : EPROTO;
-      }
-      else
-        /* Emit a debug message, but ignore it. */
-        debug (h, "server sent unexpected meta context ID %" PRIu32,
-               context_id);
-    }
+      debug (h, "server sent unexpected meta context ID %" PRIu32,
+             context_id);

     SET_NEXT_STATE(%FINISH);
   }
diff --git a/interop/dirty-bitmap.c b/interop/dirty-bitmap.c
index 329fbea..a19fb64 100644
--- a/interop/dirty-bitmap.c
+++ b/interop/dirty-bitmap.c
@@ -41,7 +41,7 @@ struct data {

 static int
 cb (void *opaque, const char *metacontext, uint64_t offset,
-    uint32_t *entries, size_t len)
+    uint32_t *entries, size_t len, int *error)
 {
   struct data *data = opaque;

@@ -50,6 +50,7 @@ cb (void *opaque, const char *metacontext, uint64_t offset,
    * the fact that qemu-nbd is compliant.
    */
   assert (offset == 0);
+  assert (!*error || (data->fail && data->count == 1 &&
*error == EPROTO));
   assert (data->count-- > 0); /* [qemu-nbd] */

   if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) {
@@ -91,7 +92,8 @@ cb (void *opaque, const char *metacontext, uint64_t offset,
   }

   if (data->fail) {
-    errno = EPROTO; /* Something NBD servers can't send */
+    /* Something NBD servers can't send */
+    *error = data->count == 1 ? EPROTO : ECONNREFUSED;
     return -1;
   }
   return 0;
@@ -147,17 +149,14 @@ main (int argc, char *argv[])
   }
   assert (data.seen_base && data.seen_dirty);

-  /* Trigger a failed callback, to prove connection stays up. No way
-   * to know which context will respond first, but we can check that
-   * the other one did not get visited.
-   */
-  data = (struct data) { .count = 1, .fail = true, };
+  /* Trigger a failed callback, to prove connection stays up. */
+  data = (struct data) { .count = 2, .fail = true, };
   if (nbd_block_status (nbd, exportsize, 0, &data, cb, 0) != -1) {
     fprintf (stderr, "unexpected block status success\n");
     exit (EXIT_FAILURE);
   }
   assert (nbd_get_errno () == EPROTO && nbd_aio_is_ready (nbd));
-  assert (data.seen_base ^ data.seen_dirty);
+  assert (data.seen_base && data.seen_dirty);

   if (nbd_pread (nbd, &c, 1, 0, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
diff --git a/lib/internal.h b/lib/internal.h
index f827957..88ad703 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -232,7 +232,7 @@ struct socket {
 };

 typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset,
-                          uint32_t *entries, size_t nr_entries);
+                          uint32_t *entries, size_t nr_entries, int *error);
 typedef int (*read_fn) (void *data, const void *buf, size_t count,
                         uint64_t offset, int *error, int status);

diff --git a/ocaml/examples/extents.ml b/ocaml/examples/extents.ml
index b49f98e..6681446 100644
--- a/ocaml/examples/extents.ml
+++ b/ocaml/examples/extents.ml
@@ -16,7 +16,8 @@ let ()    (* Read the extents and print them. *)
   let size = NBD.get_size nbd in
   NBD.block_status nbd size 0_L () (
-    fun () meta _ entries ->
+    fun () meta _ entries err ->
+      printf "err=%d\n" !err;
       if meta = "base:allocation" then (
         printf "index\tlength\tflags\n";
         for i = 0 to Array.length entries / 2 - 1 do
diff --git a/python/t/460-block-status.py b/python/t/460-block-status.py
index eeaa7b2..7e4ba2c 100644
--- a/python/t/460-block-status.py
+++ b/python/t/460-block-status.py
@@ -27,9 +27,10 @@ h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
                     "sh", script])

 entries = []
-def f (data, metacontext, offset, e):
+def f (data, metacontext, offset, e, err):
     global entries
     assert data == 42
+    assert err.value == 0
     if metacontext != "base:allocation":
         return
     entries = e
diff --git a/tests/meta-base-allocation.c b/tests/meta-base-allocation.c
index a9ddbd0..8ae215e 100644
--- a/tests/meta-base-allocation.c
+++ b/tests/meta-base-allocation.c
@@ -30,7 +30,7 @@

 static int check_extent (void *data, const char *metacontext,
                          uint64_t offset,
-                         uint32_t *entries, size_t nr_entries);
+                         uint32_t *entries, size_t nr_entries, int *error);

 int
 main (int argc, char *argv[])
@@ -128,15 +128,16 @@ main (int argc, char *argv[])
 static int
 check_extent (void *data, const char *metacontext,
               uint64_t offset,
-              uint32_t *entries, size_t nr_entries)
+              uint32_t *entries, size_t nr_entries, int *error)
 {
   size_t i;
   int id = * (int *)data;

   printf ("extent: id=%d, metacontext=%s, offset=%" PRIu64 ",
"
-          "nr_entries=%zu\n",
-          id, metacontext, offset, nr_entries);
+          "nr_entries=%zu, error=%d\n",
+          id, metacontext, offset, nr_entries, *error);

+  assert (*error == 0);
   if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) == 0) {
     for (i = 0; i < nr_entries; i += 2) {
       printf ("\t%zu\tlength=%" PRIu32 ", status=%" PRIu32
"\n",
-- 
2.20.1

ACK

-- 
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