Libguestfs - Jun 2019 - [libnbd PATCH] pread_structured: Change callback type to use Mutable error

Take advantage of the previous patch to make it easier for language
bindings to affect the exact error they want on failure, rather than
requiring them to influence errno.

Update the python test to tickle the changed bindings, and to prove
that we can at least trigger an exception, although we are still
lacking bindings for python code to access the last NBD exception and
error code gracefully (maybe we should be returning a specific
subclass of Exception that contains the last error information).
---

Applies on top of my fixes to Rich's proposal for a Mutable(Int)
callback parameter in the generator.

I validated that:

except Exception as e:
    print(e)

in 405-pread-structured.py is sufficient to produce output resembling:

nbd_pread_structured: read: command failed: Protocol error
Traceback (most recent call last):
  File "./t/405-pread-structured.py", line 48, in <module>
    buf = h.pread_structured (512, 0, 43, f, nbd.CMD_FLAG_DF)
  File "/home/eblake/libnbd/python/nbd.py", line 517, in
pread_structured
    return libnbdmod.pread_structured (self._o, count, offset, data, chunk, flag
s)
RuntimeError: nbd_pread_structured: read: command failed: Protocol error

where assigning to error.value in the callback affected the strerror
reference in the RuntimeError; but we really should be thinking about
returning a subclass of RuntimeError for programmatic access from
python rather than just scraping the generic error string.

 generator/generator                 | 28 ++++++++++++++++------------
 generator/states-reply-simple.c     |  7 ++++---
 generator/states-reply-structured.c | 21 ++++++++++++---------
 interop/structured-read.c           |  6 +++---
 lib/internal.h                      |  2 +-
 python/t/405-pread-structured.py    | 12 +++++++++++-
 tests/oldstyle.c                    |  8 ++++----
 7 files changed, 51 insertions(+), 33 deletions(-)

diff --git a/generator/generator b/generator/generator
index a9a23f4..37fa0fc 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1330,7 +1330,7 @@ protocol extensions).";
     args = [ BytesOut ("buf", "count"); UInt64
"offset";
              Opaque "data";
              Callback ("chunk", [ Opaque "data"; BytesIn
("subbuf", "count");
-                                  UInt64 "offset"; Int
"error";
+                                  UInt64 "offset"; Mutable (Int
"error");
                                   Int "status" ]);
              Flags "flags" ];
     ret = RErr;
@@ -1346,8 +1346,9 @@ additional sanity checking on the server's reply. 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>, and no
 earlier error has been detected, then the overall read command will
-fail with the same value of C<errno> left after the failing callback;
-but any further chunks will still invoke the callback.
+fail with any non-zero value stored into the callback's C<error>
+parameter (with a default of C<EPROTO>); but any further chunks will
+still invoke the callback.

 The C<chunk> function is called once per chunk of data received.  The
 C<subbuf> and C<count> parameters represent the subset of the
original
@@ -1356,25 +1357,27 @@ C<subbuf> always points within the original
C<buf>; but this guarantee
 may not extend to other language bindings). The C<offset> parameter
 represents the absolute offset at which C<subbuf> begins within the
 image (note that this is not the relative offset of C<subbuf> within
-the original buffer C<buf>). The C<error> parameter is controlled
by
-the C<status> parameter, which is one of
+the original buffer C<buf>). Changes to C<error> on output are
ignored
+unless the callback fails. The input meaning of the C<error> parameter
+is controlled by the C<status> parameter, which is one of

 =over 4

 =item C<LIBNBD_READ_DATA> = 1

-C<subbuf> was populated with C<count> bytes of data. C<error>
is the
-errno value of any earlier detected error, or zero.
+C<subbuf> was populated with C<count> bytes of data. On input,
C<error>
+contains the errno value of any earlier detected error, or zero.

 =item C<LIBNBD_READ_HOLE> = 2

-C<subbuf> represents a hole, and contains C<count> NUL bytes.
C<error>
-is the errno value of any earlier detected error, or zero.
+C<subbuf> represents a hole, and contains C<count> NUL bytes. On
input,
+C<error> contains the errno value of any earlier detected error, or zero.

 =item C<LIBNBD_READ_ERROR> = 3

-C<count> is 0, so C<subbuf> is unusable. C<error> is the
errno value
-reported by the server as occurring while reading that C<offset>.
+C<count> is 0, so C<subbuf> is unusable. On input, C<error>
contains the
+errno value reported by the server as occurring while reading that
+C<offset>, regardless if any earlier error has been detected.

 =back

@@ -1685,7 +1688,8 @@ parameters behave as documented in
C<nbd_pread>.";
              Opaque "data";
              CallbackPersist ("chunk", [ Opaque "data";
                                          BytesIn ("subbuf",
"count");
-                                         UInt64 "offset"; Int
"error";
+                                         UInt64 "offset";
+                                         Mutable (Int "error");
                                          Int "status" ]);
              Flags "flags" ];
     ret = RInt64;
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 25eab9d..cab72d6 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -61,11 +61,12 @@
     /* guaranteed by START */
     assert (cmd);
     if (cmd->cb.fn.read) {
+      int error = 0;
+
       assert (cmd->error == 0);
-      errno = 0;
       if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data, cmd->count,
-                           cmd->offset, 0, LIBNBD_READ_DATA) == -1)
-        cmd->error = errno ? errno : EPROTO;
+                           cmd->offset, &error, LIBNBD_READ_DATA) == -1)
+        cmd->error = error ? error : EPROTO;
     }

     SET_NEXT_STATE (%^FINISH_COMMAND);
diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 24bead6..fa11dd6 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -292,15 +292,16 @@
         return -1;
       }
       if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read) {
+        int scratch = error;
+
         /* Different from successful reads: inform the callback about the
          * current error rather than any earlier one. If the callback fails
          * without setting errno, then use the server's error below.
          */
-        errno = 0;
         if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset -
cmd->offset),
-                             0, offset, error, LIBNBD_READ_ERROR) == -1)
+                             0, offset, &scratch, LIBNBD_READ_ERROR) == -1)
           if (cmd->error == 0)
-            cmd->error = errno;
+            cmd->error = scratch;
       }
     }

@@ -381,12 +382,13 @@

     assert (cmd); /* guaranteed by CHECK */
     if (cmd->cb.fn.read) {
-      errno = 0;
+      int error = cmd->error;
+
       if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset -
cmd->offset),
-                           length - sizeof offset, offset, cmd->error,
+                           length - sizeof offset, offset, &error,
                            LIBNBD_READ_DATA) == -1)
         if (cmd->error == 0)
-          cmd->error = errno ? errno : EPROTO;
+          cmd->error = error ? error : EPROTO;
     }

     SET_NEXT_STATE (%FINISH);
@@ -441,12 +443,13 @@
      */
     memset (cmd->data + offset, 0, length);
     if (cmd->cb.fn.read) {
-      errno = 0;
+      int error = cmd->error;
+
       if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + offset, length,
-                           cmd->offset + offset, cmd->error,
+                           cmd->offset + offset, &error,
                            LIBNBD_READ_HOLE) == -1)
         if (cmd->error == 0)
-          cmd->error = errno ? errno : EPROTO;
+          cmd->error = error ? error : EPROTO;
     }

     SET_NEXT_STATE(%FINISH);
diff --git a/interop/structured-read.c b/interop/structured-read.c
index cf8b893..46a7a80 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -47,7 +47,7 @@ struct data {

 static int
 read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset,
-         int error, int status)
+         int *error, int status)
 {
   struct data *data = opaque;
   const char *buf = bufv;
@@ -55,7 +55,7 @@ read_cb (void *opaque, const void *bufv, size_t count,
uint64_t offset,
   /* The NBD spec allows chunks to be reordered; we are relying on the
    * fact that qemu-nbd does not do so.
    */
-  assert (!error || (data->fail && data->count == 1));
+  assert (!*error || (data->fail && data->count == 1 &&
*error == EPROTO));
   assert (data->count-- > 0);

   switch (status) {
@@ -93,7 +93,7 @@ read_cb (void *opaque, const void *bufv, size_t count,
uint64_t offset,

   if (data->fail) {
     /* Something NBD servers can't send */
-    errno = data->count == 1 ? EPROTO : ECONNREFUSED;
+    *error = data->count == 1 ? EPROTO : ECONNREFUSED;
     return -1;
   }
   return 0;
diff --git a/lib/internal.h b/lib/internal.h
index 7d87fa4..f827957 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -234,7 +234,7 @@ struct socket {
 typedef int (*extent_fn) (void *data, const char *metacontext, uint64_t offset,
                           uint32_t *entries, size_t nr_entries);
 typedef int (*read_fn) (void *data, const void *buf, size_t count,
-                        uint64_t offset, int error, int status);
+                        uint64_t offset, int *error, int status);

 struct command_cb {
   void *opaque;
diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py
index 1bfa162..9ed2090 100644
--- a/python/t/405-pread-structured.py
+++ b/python/t/405-pread-structured.py
@@ -16,6 +16,7 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA

 import nbd
+import errno

 h = nbd.NBD ()
 h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
@@ -24,10 +25,11 @@ h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
 expected =
b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x08\x00\x00\x00\x00\x00\x00\x00\x10\x00\x00\x00\x00\x00\x00\x00\x18\x00\x00\x00\x00\x00\x00\x00
\x00\x00\x00\x00\x00\x00\x00(\x00\x00\x00\x00\x00\x00\x000\x00\x00\x00\x00\x00\x00\x008\x00\x00\x00\x00\x00\x00\x00@\x00\x00\x00\x00\x00\x00\x00H\x00\x00\x00\x00\x00\x00\x00P\x00\x00\x00\x00\x00\x00\x00X\x00\x00\x00\x00\x00\x00\x00`\x00\x00\x00\x00\x00\x00\x00h\x00\x00\x00\x00\x00\x00\x00p\x00\x00\x00\x00\x00\x00\x00x\x00\x00\x00\x00\x00\x00\x00\x80\x00\x00\x00\x00\x00\x00\x00\x88\x00\x00\x00\x00\x00\x00\x00\x90\x00\x00\x00\x00\x00\x00\x00\x98\x00\x00\x00\x00\x00\x00\x00\xa0\x00\x00\x00\x00\x00\x00\x00\xa8\x00\x00\x00\x00\x00\x00\x00\xb0\x00\x00\x00\x00\x00\x00\x00\xb8\x00\x00\x00\x00\x00\x00\x00\xc0\x00\x00\x00\x00\x00\x00\x00\xc8\x00\x00\x00\x00\x00\x00\x00\xd0\x00\x00\x00\x00\x00\x00\x00\xd8\x00\x00\x00\x00\x00\x00\x00\xe0\x00\x00\x00\x00\x00\x00\x00\xe8\x00\x00\x00\x00\x00\x00\x00\xf0\x00\x00\x00\x00\x00\x00\x00\xf8\x00\x00\x00\x00\x00\x00\x01\x00\x00\x00\x00\x00\x00\x00\x01\x08\x00\x00\x00\x00\x00\x00\x01\x10\x00\x00\x00\x00\x00\x00\x01\x18\x00\x00\x00\x00\x00\x00\x01
\x00\x00\x00\x00\x00\x00\x01(\x00\x00\x00\x00\x00\x00\x010\x00\x00\x00\x00\x00\x00\x018\x00\x00\x00\x00\x00\x00\x01@\x00\x00\x00\x00\x00\x00\x01H\x00\x00\x00\x00\x00\x00\x01P\x00\x00\x00\x00\x00\x00\x01X\x00\x00\x00\x00\x00\x00\x01`\x00\x00\x00\x00\x00\x00\x01h\x00\x00\x00\x00\x00\x00\x01p\x00\x00\x00\x00\x00\x00\x01x\x00\x00\x00\x00\x00\x00\x01\x80\x00\x00\x00\x00\x00\x00\x01\x88\x00\x00\x00\x00\x00\x00\x01\x90\x00\x00\x00\x00\x00\x00\x01\x98\x00\x00\x00\x00\x00\x00\x01\xa0\x00\x00\x00\x00\x00\x00\x01\xa8\x00\x00\x00\x00\x00\x00\x01\xb0\x00\x00\x00\x00\x00\x00\x01\xb8\x00\x00\x00\x00\x00\x00\x01\xc0\x00\x00\x00\x00\x00\x00\x01\xc8\x00\x00\x00\x00\x00\x00\x01\xd0\x00\x00\x00\x00\x00\x00\x01\xd8\x00\x00\x00\x00\x00\x00\x01\xe0\x00\x00\x00\x00\x00\x00\x01\xe8\x00\x00\x00\x00\x00\x00\x01\xf0\x00\x00\x00\x00\x00\x00\x01\xf8'

 def f (data, buf2, offset, err, s):
+    assert err.value == 0
+    err.value = errno.EPROTO
     assert data == 42
     assert buf2 == expected
     assert offset == 0
-    assert err == 0
     assert s == nbd.READ_DATA

 buf = h.pread_structured (512, 0, 42, f)
@@ -41,3 +43,11 @@ buf = h.pread_structured (512, 0, 42, f, nbd.CMD_FLAG_DF)
 print ("%r" % buf)

 assert buf == expected
+
+try:
+    buf = h.pread_structured (512, 0, 43, f, nbd.CMD_FLAG_DF)
+    assert False
+except Exception:
+    # Need a way for python to access last NBD error...
+    # assert nbd.get_errno == errno.EPROTO
+    pass
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index 38f5130..0207bf8 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -36,7 +36,7 @@ static const char *progname;

 static int
 pread_cb (void *data, const void *buf, size_t count, uint64_t offset,
-          int error, int status)
+          int *error, int status)
 {
   int *calls = data;
   ++*calls;
@@ -49,7 +49,7 @@ pread_cb (void *data, const void *buf, size_t count, uint64_t
offset,
     fprintf (stderr, "%s: callback called with wrong offset\n",
progname);
     exit (EXIT_FAILURE);
   }
-  if (error != 0) {
+  if (*error != 0) {
     fprintf (stderr, "%s: callback called with unexpected error\n",
progname);
     exit (EXIT_FAILURE);
   }
@@ -64,7 +64,7 @@ pread_cb (void *data, const void *buf, size_t count, uint64_t
offset,
   }

   if (*calls > 1) {
-    errno = EPROTO; /* Something NBD servers can't send */
+    *error = ECONNREFUSED; /* Something NBD servers can't send */
     return -1;
   }

@@ -143,7 +143,7 @@ main (int argc, char *argv[])
     fprintf (stderr, "%s: expected failure from callback\n",
argv[0]);
     exit (EXIT_FAILURE);
   }
-  if (nbd_get_errno () != EPROTO) {
+  if (nbd_get_errno () != ECONNREFUSED) {
     fprintf (stderr, "%s: wrong errno value after failed callback\n",
argv[0]);
     exit (EXIT_FAILURE);
   }
-- 
2.20.1

ACK

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