Libguestfs - Jun 2019 - Re: [libnbd PATCH v2 2/5] states: Wire in a read callback

Since v1:
- rebase to applied patches
- split out support for Int in callbacks
- sort of test that callbacks work in OCaml (see comment in patch 5)
- rename API to nbd_pread_structured
- expose error as explicit parameter to callback

Eric Blake (5):
  generator: Allow Int in callbacks
  states: Wire in a read callback
  states: Add nbd_pread_structured API
  states: Add tests for nbd_pread_structured
  states: Add DF flag support for pread

 .gitignore                          |   1 +
 generator/generator                 | 140 +++++++++++++++++++--
 generator/states-reply-simple.c     |  15 ++-
 generator/states-reply-structured.c |  44 ++++++-
 interop/Makefile.am                 |  11 +-
 interop/structured-read.c           | 183 ++++++++++++++++++++++++++++
 interop/structured-read.sh          |  57 +++++++++
 lib/flags.c                         |  11 ++
 lib/internal.h                      |   4 +-
 lib/nbd-protocol.h                  |   1 +
 lib/protocol.c                      |   1 +
 lib/rw.c                            |  42 +++++++
 python/t/405-pread-structured.py    |  43 +++++++
 tests/oldstyle.c                    |  78 +++++++++++-
 14 files changed, 611 insertions(+), 20 deletions(-)
 create mode 100644 interop/structured-read.c
 create mode 100755 interop/structured-read.sh
 create mode 100644 python/t/405-pread-structured.py

-- 
2.20.1

An upcoming patch to add callbacks during structured reads wants to
expose Int as a callback parameter. It's time to wire that up.
---
 generator/generator | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/generator/generator b/generator/generator
index e1a97a5..2d1a4e5 100755
--- a/generator/generator
+++ b/generator/generator
@@ -3260,7 +3260,8 @@ let print_python_binding name { args; ret }             
pr "  PyObject *py_%s = PyList_New (%s);\n" n len;
             pr "  for (size_t i = 0; i < %s; ++i)\n" len;
             pr "    PyList_SET_ITEM (py_%s, i, PyLong_FromUnsignedLong
(%s[i]));\n" n n
-         | BytesIn _ -> ()
+         | BytesIn _
+         | Int _ -> ()
          | Opaque n ->
             pr "  struct %s_%s_data *_data = %s;\n" name cb_name n
          | String n
@@ -3268,7 +3269,7 @@ let print_python_binding name { args; ret }           (*
The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr "\n";
@@ -3278,13 +3279,14 @@ let print_python_binding name { args; ret }          
function
          | ArrayAndLen (UInt32 n, len) -> pr " \"O\""
          | BytesIn (n, len) -> pr " \"y#\""
+         | Int n -> pr " \"i\""
          | Opaque n -> pr " \"O\""
          | String n -> pr " \"s\""
          | UInt64 n -> pr " \"K\""
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr " \")\"";
@@ -3293,12 +3295,13 @@ let print_python_binding name { args; ret }           |
ArrayAndLen (UInt32 n, _) -> pr ", py_%s" n
          | BytesIn (n, len) -> pr ", %s, (int) %s" n len
          | Opaque _ -> pr ", _data->data"
+         | Int n
          | String n
          | UInt64 n -> pr ", %s" n
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr ");\n";
@@ -3328,13 +3331,14 @@ let print_python_binding name { args; ret }           |
ArrayAndLen (UInt32 n, _) ->
             pr "  Py_DECREF (py_%s);\n" n
          | BytesIn _
+         | Int _
+         | Opaque _
          | String _
-         | UInt64 _
-         | Opaque _ -> ()
+         | UInt64 _ -> ()
          (* The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;
        pr "  return ret;\n";
@@ -3985,13 +3989,13 @@ let print_ocaml_binding (name, { args; ret })          
List.map (
            function
            | ArrayAndLen (UInt32 n, _) | BytesIn (n, _)
-           | String n | UInt64 n | Opaque n ->
+           | Int n | Opaque n | String n | UInt64 n ->
               n ^ "v"
            (* The following not yet implemented for callbacks XXX *)
            | ArrayAndLen _ | Bool _ | BytesOut _
            | BytesPersistIn _ | BytesPersistOut _
            | Callback _ | CallbackPersist _
-           | Flags _ | Int _ | Int64 _ | Path _
+           | Flags _ | Int64 _ | Path _
            | SockAddrAndLen _ | StringList _
            | UInt _ | UInt32 _ -> assert false
          ) args in
@@ -4017,6 +4021,8 @@ let print_ocaml_binding (name, { args; ret })           |
BytesIn (n, len) ->
             pr "  %sv = caml_alloc_string (%s);\n" n len;
             pr "  memcpy (String_val (%sv), %s, %s);\n" n n len
+         | Int n ->
+            pr "  %sv = Val_int (%s);\n" n n
          | String n ->
             pr "  %sv = caml_copy_string (%s);\n" n n
          | UInt64 n ->
@@ -4028,7 +4034,7 @@ let print_ocaml_binding (name, { args; ret })           (*
The following not yet implemented for callbacks XXX *)
          | ArrayAndLen _ | Bool _ | BytesOut _
          | BytesPersistIn _ | BytesPersistOut _ | Callback _ | CallbackPersist
_
-         | Flags _ | Int _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
+         | Flags _ | Int64 _ | Path _ | SockAddrAndLen _ | StringList _
          | UInt _ | UInt32 _ -> assert false
        ) args;

-- 
2.20.1

When a server supports structured reads, knowing where the holes are
in the responses can aid the client's behavior.  Furthermore, since
the server can send data out of order or interleaved with other
replies, a client may be able to start operating on the partial
results as soon as they are available rather than waiting for the
entire buffer to be reconstructed.  As such, it makes sense to have an
optional callback invoked each time we finish reading a chunk (for
simple replies, we behave as if everything was a single data chunk).

This also requires special handling of NBD_REPLY_TYPE_ERROR_OFFSET, as
that is the one situation where we want to call the callback to inform
them that the server pointed out a specific offset for the error - a
client can utilize that knowledge to form a wrapper that permits short
reads (rather than all-or-nothing failures). But as that is the only
error with an offset, all other errors continue to affect only the
final pread return value without any use of the callback.

If any callback fails, and if no prior error was set, then the
callback's failure becomes the failure reason for the overall
read. (Note that this is different from the block status callback,
which for now quits using the callback on subsequent chunks if an
earlier chunk failed - we may decide to change one or the other for
consistency before the API is stable.)

Nothing actually passes a callback function yet, so for now this is no
functional change; but this will make it possible for the next patch
to add an 'nbd_aio_pread_structred' API.
---
 generator/generator                 |  4 +++
 generator/states-reply-simple.c     | 15 +++++++++-
 generator/states-reply-structured.c | 44 +++++++++++++++++++++++++++--
 lib/internal.h                      |  4 ++-
 4 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/generator/generator b/generator/generator
index 2d1a4e5..b408509 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1934,6 +1934,10 @@ let constants = [
   "CMD_FLAG_FUA",        1 lsl 0;
   "CMD_FLAG_NO_HOLE",    1 lsl 1;
   "CMD_FLAG_REQ_ONE",    1 lsl 3;
+
+  "READ_DATA",           1;
+  "READ_HOLE",           2;
+  "READ_ERROR",          3;
 ]

 (*----------------------------------------------------------------------*)
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 935f6d2..87622a0 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -49,9 +49,22 @@
   return 0;

  REPLY.SIMPLE_REPLY.RECV_READ_PAYLOAD:
+  struct command_in_flight *cmd = h->reply_cmd;
+
   switch (recv_into_rbuf (h)) {
   case -1: SET_NEXT_STATE (%.DEAD); return -1;
-  case 0:  SET_NEXT_STATE (%^FINISH_COMMAND);
+  case 0:
+    /* guaranteed by START */
+    assert (cmd);
+    if (cmd->cb.fn.read) {
+      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;
+    }
+
+    SET_NEXT_STATE (%^FINISH_COMMAND);
   }
   return 0;

diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
index 9dcc898..f1d421d 100644
--- a/generator/states-reply-structured.c
+++ b/generator/states-reply-structured.c
@@ -228,13 +228,16 @@
     type = be16toh (h->sbuf.sr.structured_reply.type);

     assert (cmd); /* guaranteed by CHECK */
+    error = nbd_internal_errno_of_nbd_error (error);

     /* The spec requires the server to send a non-zero error */
     if (error == NBD_SUCCESS)
       debug (h, "server forgot to set error; using EINVAL");
     error = nbd_internal_errno_of_nbd_error (error ? error : EINVAL);

-    /* Sanity check that any error offset is in range */
+    /* Sanity check that any error offset is in range, then invoke
+     * user callback if present.
+     */
     if (type == NBD_REPLY_TYPE_ERROR_OFFSET) {
       offset = be64toh (h->sbuf.sr.payload.error.offset);
       if (offset < cmd->offset || offset >= cmd->offset +
cmd->count) {
@@ -246,6 +249,17 @@
                    offset, cmd->offset, cmd->count);
         return -1;
       }
+      if (cmd->cb.fn.read) {
+        /* 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)
+          if (cmd->error == 0)
+            cmd->error = errno;
+      }
     }

     /* Preserve first error encountered */
@@ -313,9 +327,27 @@
   return 0;

  REPLY.STRUCTURED_REPLY.RECV_OFFSET_DATA_DATA:
+  struct command_in_flight *cmd = h->reply_cmd;
+  uint64_t offset;
+  uint32_t length;
+
   switch (recv_into_rbuf (h)) {
   case -1: SET_NEXT_STATE (%.DEAD); return -1;
-  case 0:  SET_NEXT_STATE (%FINISH);
+  case 0:
+    length = be32toh (h->sbuf.sr.structured_reply.length);
+    offset = be64toh (h->sbuf.sr.payload.offset_data.offset);
+
+    assert (cmd); /* guaranteed by CHECK */
+    if (cmd->cb.fn.read) {
+      errno = 0;
+      if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + (offset -
cmd->offset),
+                           length - sizeof offset, offset, cmd->error,
+                           LIBNBD_READ_DATA) == -1)
+        if (cmd->error == 0)
+          cmd->error = errno ? errno : EPROTO;
+    }
+
+    SET_NEXT_STATE (%FINISH);
   }
   return 0;

@@ -370,6 +402,14 @@
      * them as an extension, and this works even when length == 0.
      */
     memset (cmd->data + offset, 0, length);
+    if (cmd->cb.fn.read) {
+      errno = 0;
+      if (cmd->cb.fn.read (cmd->cb.opaque, cmd->data + offset, length,
+                           cmd->offset + offset, cmd->error,
+                           LIBNBD_READ_HOLE) == -1)
+        if (cmd->error == 0)
+          cmd->error = errno ? errno : EPROTO;
+    }

     SET_NEXT_STATE(%FINISH);
   }
diff --git a/lib/internal.h b/lib/internal.h
index 3756fac..b79c7a9 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -233,12 +233,14 @@ 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);

 struct command_cb {
   void *opaque;
   union {
     extent_fn extent;
-    /* More to come */
+    read_fn read;
   } fn;
 };

-- 
2.20.1

Time to wire up user access to react to structured reads as the chunks
come in, exposing the framework added in the last patch.  I chose to
use Int for the callback status rather than an enum for ease of wiring
things up to the various language bindings.

It's easy to test callback behavior for LIBNBD_READ_DATA/HOLE (the
next patch will add interop tests with qemu-nbd), but at present, I
don't know of any server that responds with
NBD_REPLY_TYPE_ERROR_OFFSET, which in turn makes testing
LIBNBD_READ_ERROR difficult. I used the following hack on top of
qemu.git commit d1bf88e5 to trigger an offset error for local testing:

| diff --git i/nbd/server.c w/nbd/server.c
| index 10faedcfc55d..804dced0bc8d 100644
| --- i/nbd/server.c
| +++ w/nbd/server.c
| @@ -1838,12 +1838,30 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
|
|              trace_nbd_co_send_structured_read_hole(handle, offset + progress,
|                                                     pnum);
| -            set_be_chunk(&chunk.h, final ? NBD_REPLY_FLAG_DONE : 0,
| +            set_be_chunk(&chunk.h, 0,
|                           NBD_REPLY_TYPE_OFFSET_HOLE,
|                           handle, sizeof(chunk) - sizeof(chunk.h));
|              stq_be_p(&chunk.offset, offset + progress);
|              stl_be_p(&chunk.length, pnum);
|              ret = nbd_co_send_iov(client, iov, 1, errp);
| +            if (ret == 0) {
| +                NBDStructuredError chunk;
| +                int64_t off;
| +                struct iovec iov[] = {
| +                    {.iov_base = &chunk, .iov_len = sizeof(chunk)},
| +                    {.iov_base = (char*)"MSG", .iov_len = 3},
| +                    {.iov_base = &off, .iov_len = sizeof(off)},
| +                };
| +
| +                set_be_chunk(&chunk.h, final ? NBD_REPLY_FLAG_DONE : 0,
| +                             NBD_REPLY_TYPE_ERROR_OFFSET,
| +                             handle, sizeof(chunk) - sizeof(chunk.h) +
| +                             3 + sizeof(off));
| +                stl_be_p(&chunk.error, NBD_EPERM);
| +                stw_be_p(&chunk.message_length, 3);
| +                stq_be_p(&off, offset + progress);
| +                ret = nbd_co_send_iov(client, iov, 3, errp);
| +            }
|          } else {
|              ret = blk_pread(exp->blk, offset + progress +
exp->dev_offset,
|                              data + progress, pnum);
---
 generator/generator | 94 ++++++++++++++++++++++++++++++++++++++++++++-
 lib/rw.c            | 32 +++++++++++++++
 2 files changed, 125 insertions(+), 1 deletion(-)

diff --git a/generator/generator b/generator/generator
index b408509..cf9876f 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1305,7 +1305,79 @@ Issue a read command to the NBD server for the range
starting
 at C<offset> and ending at C<offset> + C<count> - 1.  NBD
 can only read all or nothing using this call.  The call
 returns when the data has been read fully into C<buf> or there is an
-error.
+error.  See also C<nbd_pread_structured>, if finer visibility is
+required into the server's replies.
+
+The C<flags> parameter must be C<0> for now (it exists for future
NBD
+protocol extensions).";
+  };
+
+  "pread_structured", {
+    default_call with
+    args = [ BytesOut ("buf", "count"); UInt64
"offset";
+             Opaque "data";
+             Callback ("chunk", [ Opaque "data"; BytesIn
("subbuf", "count");
+                                  UInt64 "offset"; Int
"error";
+                                  Int "status" ]);
+             Flags "flags" ];
+    ret = RErr;
+    permitted_states = [ Connected ];
+    shortdesc = "read from the NBD server";
+    longdesc = "\
+Issue a read command to the NBD server for the range starting
+at C<offset> and ending at C<offset> + C<count> - 1.  The
server's
+response may be subdivided into chunks which may arrive out of order
+before reassembly into the original buffer; the C<chunk> callback
+is used for notification after each chunk arrives, and may perform
+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.
+
+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
+buffer which has just been populated by results from the server (in C,
+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
+
+=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.
+
+=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.
+
+=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>.
+
+=back
+
+Future NBD extensions may permit other values for C<status>, but those
+will not be returned to a client that has not opted in to requesting
+such extensions. If the server is non-compliant, it is possible for
+the C<chunk> function to be called more times than you expect or with
+C<count> 0 for C<LIBNBD_READ_DATA> or C<LIBNBD_READ_HOLE>. It
is also
+possible that the C<chunk> function is not called at all (in
+particular, C<LIBNBD_READ_ERROR> is used only when an error is
+associated with a particular offset, and not when the server reports a
+generic error), but you are guaranteed that the callback was called at
+least once if the overall read succeeds. Libnbd does not validate that
+the server obeyed the requirement that a read call must not have
+overlapping chunks and must not succeed without enough chunks to cover
+the entire request.

 The C<flags> parameter must be C<0> for now (it exists for future
NBD
 protocol extensions).";
@@ -1591,6 +1663,26 @@ C<buf> is valid until the command has completed. 
Other
 parameters behave as documented in C<nbd_pread>.";
   };

+  "aio_pread_structured", {
+    default_call with
+    args = [ BytesPersistOut ("buf", "count"); UInt64
"offset";
+             Opaque "data";
+             CallbackPersist ("chunk", [ Opaque "data";
+                                         BytesIn ("subbuf",
"count");
+                                         UInt64 "offset"; Int
"error";
+                                         Int "status" ]);
+             Flags "flags" ];
+    ret = RInt64;
+    permitted_states = [ Connected ];
+    shortdesc = "read from the NBD server";
+    longdesc = "\
+Issue a read command to the NBD server.  This returns the
+unique positive 64 bit handle for this command, or C<-1> on
+error.  To check if the command completed, call
+C<nbd_aio_command_completed>.  Parameters behave as documented
+in C<nbd_pread_structured>.";
+  };
+
   "aio_pwrite", {
     default_call with
     args = [ BytesPersistIn ("buf", "count"); UInt64
"offset"; Flags "flags" ];
diff --git a/lib/rw.c b/lib/rw.c
index dc81c57..24dbc4e 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -55,6 +55,22 @@ nbd_unlocked_pread (struct nbd_handle *h, void *buf,
   return wait_for_command (h, ch);
 }

+/* Issue a read command with callbacks and wait for the reply. */
+int
+nbd_unlocked_pread_structured (struct nbd_handle *h, void *buf,
+                               size_t count, uint64_t offset,
+                               void *opaque, read_fn read, uint32_t flags)
+{
+  int64_t ch;
+
+  ch = nbd_unlocked_aio_pread_structured (h, buf, count, offset,
+                                          opaque, read, flags);
+  if (ch == -1)
+    return -1;
+
+  return wait_for_command (h, ch);
+}
+
 /* Issue a write command and wait for the reply. */
 int
 nbd_unlocked_pwrite (struct nbd_handle *h, const void *buf,
@@ -231,6 +247,22 @@ nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
                                       buf, NULL);
 }

+int64_t
+nbd_unlocked_aio_pread_structured (struct nbd_handle *h, void *buf,
+                                   size_t count, uint64_t offset,
+                                   void *opaque, read_fn read, uint32_t flags)
+{
+  struct command_cb cb = { .opaque = opaque, .fn.read = read, };
+
+  if (flags != 0) {
+    set_error (EINVAL, "invalid flag: %" PRIu32, flags);
+    return -1;
+  }
+
+  return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
+                                      buf, &cb);
+}
+
 int64_t
 nbd_unlocked_aio_pwrite (struct nbd_handle *h, const void *buf,
                          size_t count, uint64_t offset,
-- 
2.20.1

With a new enough qemu-nbd, we can test that nbd_pread_structured for
sane operation with interop/structured-read.

It is also possible to test that the callback gets used sanely even
for a connection lacks structured replies, by using nbdkit in
tests/oldstyle.

---

Not done here, because I'm not comfortable enough with OCaml, and
because we don't have any pre-existing ocaml/examples using pread
for doing actual buffer content validation. However, I did experiment
enough to prove that this change:

| diff --git i/ocaml/examples/extents.ml w/ocaml/examples/extents.ml
| index b49f98e..78e6c4d 100644
| --- i/ocaml/examples/extents.ml
| +++ w/ocaml/examples/extents.ml
| @@ -30,4 +30,12 @@ let () |            printf "%d:\t%ld\t%s\n" i
entries.(i*2) flags
|          done
|        )
| -  )
| +  );
| +
| +  let buf = Bytes.create 512 in
| +  NBD.pread_structured nbd buf 1024_L () (
| +    fun () sub offs err stat ->
| +      let l = Bytes.length sub in
| +      printf "%d %Ld %d %d\n" l offs err stat
| +  );
| +  printf "done\n"

was sufficient to get './run ocaml/examples/extents.bc' to output
the desired '512 1024 0 1', so it looks like the callback is wired
up correctly.
---
 .gitignore                       |   1 +
 interop/Makefile.am              |  11 +-
 interop/structured-read.c        | 166 +++++++++++++++++++++++++++++++
 interop/structured-read.sh       |  57 +++++++++++
 python/t/405-pread-structured.py |  37 +++++++
 tests/oldstyle.c                 |  78 ++++++++++++++-
 6 files changed, 345 insertions(+), 5 deletions(-)
 create mode 100644 interop/structured-read.c
 create mode 100755 interop/structured-read.sh
 create mode 100644 python/t/405-pread-structured.py

diff --git a/.gitignore b/.gitignore
index 30438c1..ea496ac 100644
--- a/.gitignore
+++ b/.gitignore
@@ -53,6 +53,7 @@ Makefile.in
 /interop/interop-qemu-nbd
 /interop/interop-qemu-nbd-tls-certs
 /interop/interop-qemu-nbd-tls-psk
+/interop/structured-read
 /lib/api.c
 /lib/libnbd.pc
 /lib/libnbd.syms
diff --git a/interop/Makefile.am b/interop/Makefile.am
index eb4b52b..6e1156d 100644
--- a/interop/Makefile.am
+++ b/interop/Makefile.am
@@ -47,10 +47,12 @@ check_PROGRAMS += \
 	interop-qemu-nbd \
 	interop-qemu-nbd-tls-certs \
 	interop-qemu-nbd-tls-psk \
-	dirty-bitmap
+	dirty-bitmap \
+	structured-read
 TESTS += \
 	interop-qemu-nbd \
-	dirty-bitmap.sh
+	dirty-bitmap.sh \
+	structured-read.sh

 # tls tests assume the pre-existence of files created in ../tests/Makefile.am,
 # so we can only run them under the same conditions used there
@@ -107,6 +109,11 @@ dirty_bitmap_CPPFLAGS = -I$(top_srcdir)/include
 dirty_bitmap_CFLAGS = $(WARNINGS_CFLAGS)
 dirty_bitmap_LDADD = $(top_builddir)/lib/libnbd.la

+structured_read_SOURCES = structured-read.c
+structured_read_CPPFLAGS = -I$(top_srcdir)/include
+structured_read_CFLAGS = $(WARNINGS_CFLAGS)
+structured_read_LDADD = $(top_builddir)/lib/libnbd.la
+
 endif HAVE_QEMU_NBD

 check-valgrind:
diff --git a/interop/structured-read.c b/interop/structured-read.c
new file mode 100644
index 0000000..ea47254
--- /dev/null
+++ b/interop/structured-read.c
@@ -0,0 +1,166 @@
+/* NBD client library in userspace
+ * Copyright (C) 2013-2019 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
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/* Test structured reply read callback. */
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <assert.h>
+#include <stdbool.h>
+#include <errno.h>
+
+#include <libnbd.h>
+
+static const char *unixsocket;
+
+/* Depends on structured-read.sh setting things up so that qemu-nbd
+ * exposes an image with a 512-byte hole at offset 2048 followed by a
+ * 512-byte data section containing all '1' bytes at offset 2560
+ * (non-zero offsets to test that everything is calculated correctly).
+ */
+static char rbuf[1024];
+
+struct data {
+  //XXX  bool df;    /* input: true if DF flag was passed to request */
+  int count;       /* input: count of expected remaining calls */
+  bool fail;       /* input: true to return failure */
+  bool seen_hole;  /* output: true if hole encountered */
+  bool seen_data;  /* output: true if data encountered */
+};
+
+static int
+read_cb (void *opaque, const void *bufv, size_t count, uint64_t offset,
+         int error, int status)
+{
+  struct data *data = opaque;
+  const char *buf = bufv;
+
+  /* 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 (data->count-- > 0);
+
+  switch (status) {
+  case LIBNBD_READ_DATA:
+    // XXX if (df...)
+    assert (buf == rbuf + 512);
+    assert (count == 512);
+    assert (offset == 2048 + 512);
+    assert (buf[0] == 1 && memcmp (buf, buf + 1, 511) == 0);
+    assert (!data->seen_data);
+    data->seen_data = true;
+    break;
+  case LIBNBD_READ_HOLE:
+    assert (buf == rbuf);
+    assert (count == 512);
+    assert (offset == 2048);
+    assert (buf[0] == 0 && memcmp (buf, buf + 1, 511) == 0);
+    assert (!data->seen_hole);
+    data->seen_hole = true;
+    break;
+  case LIBNBD_READ_ERROR:
+    /* For now, qemu-nbd cannot provoke this status. */
+  default:
+    assert (false);
+  }
+
+  if (data->fail) {
+    /* Something NBD servers can't send */
+    errno = data->count == 1 ? EPROTO : ECONNREFUSED;
+    return -1;
+  }
+  return 0;
+}
+
+int
+main (int argc, char *argv[])
+{
+  struct nbd_handle *nbd;
+  int64_t exportsize;
+  struct data data;
+  char c;
+
+  if (argc != 2) {
+    fprintf (stderr, "%s unixsocket\n", argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  unixsocket = argv[1];
+
+  nbd = nbd_create ();
+  if (nbd == NULL) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  if (nbd_connect_unix (nbd, unixsocket) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  exportsize = nbd_get_size (nbd);
+  if (exportsize == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  if (exportsize != 3072) {
+    fprintf (stderr, "unexpected file size\n");
+    exit (EXIT_FAILURE);
+  }
+
+  memset (rbuf, 2, sizeof rbuf);
+  data = (struct data) { .count = 2, };
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, &data, read_cb,
+                            0) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  assert (data.seen_data && data.seen_hole);
+
+  // XXX Repeat with DF flag
+
+  /* Trigger a failed callback, to prove connection stays up. With
+   * reads, all chunks trigger a callback even after failure, but the
+   * first errno sticks.
+   */
+  memset (rbuf, 2, sizeof rbuf);
+  data = (struct data) { .count = 2, .fail = true, };
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, &data, read_cb,
+                            0) != -1) {
+    fprintf (stderr, "unexpected pread callback success\n");
+    exit (EXIT_FAILURE);
+  }
+  assert (nbd_get_errno () == EPROTO && nbd_aio_is_ready (nbd));
+  assert (data.seen_data && data.seen_hole);
+
+  if (nbd_pread (nbd, &c, 1, 0, 0) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  if (nbd_shutdown (nbd) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  nbd_close (nbd);
+
+  exit (EXIT_SUCCESS);
+}
diff --git a/interop/structured-read.sh b/interop/structured-read.sh
new file mode 100755
index 0000000..15a81c0
--- /dev/null
+++ b/interop/structured-read.sh
@@ -0,0 +1,57 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2019 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
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+# Test structured read callbacks.
+
+source ../tests/functions.sh
+set -e
+set -x
+
+requires qemu-img --version
+requires qemu-io --version
+requires qemu-nbd --version
+
+files="structured-read.sock structured-read.qcow2"
+rm -f $files
+cleanup_fn rm -f $files
+
+# Create file with cluster size 512 and contents all 1 except for single
+# 512-byte hole at offset 2048
+qemu-img create -f qcow2 -o cluster_size=512,compat=v3 structured-read.qcow2 3k
+qemu-io -d unmap -f qcow2 -c 'w -P 1 0 3k' -c 'w -zu 2k 512' \
+	structured-read.qcow2
+
+qemu-nbd -k $PWD/structured-read.sock -f qcow2 structured-read.qcow2 &
+qemu_pid=$!
+cleanup_fn kill $qemu_pid
+
+# qemu-nbd --pid not available before 4.1, so ...
+for ((i = 0; i < 300; i++)); do
+  if [ -r $PWD/structured-read.sock ]; then
+    break
+  fi
+  kill -s 0 $qemu_pid 2>/dev/null
+  if test $? != 0; then
+    echo "qemu-nbd unexpectedly quit" 2>&1
+    exit 1
+  fi
+  sleep 0.1
+done
+
+# Run the test.
+$VG ./structured-read structured-read.sock
diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py
new file mode 100644
index 0000000..51f0144
--- /dev/null
+++ b/python/t/405-pread-structured.py
@@ -0,0 +1,37 @@
+# libnbd Python bindings
+# Copyright (C) 2010-2019 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+import nbd
+
+h = nbd.NBD ()
+h.connect_command (["nbdkit", "-s",
"--exit-with-parent", "-v",
+                    "pattern", "size=512"])
+
+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 data == 42
+    assert buf2 == expected
+    assert offset == 0
+    assert err == 0
+    assert s == nbd.READ_DATA
+
+buf = h.pread_structured (512, 0, 42, f)
+
+print ("%r" % buf)
+
+assert buf == expected
diff --git a/tests/oldstyle.c b/tests/oldstyle.c
index a0b594c..38f5130 100644
--- a/tests/oldstyle.c
+++ b/tests/oldstyle.c
@@ -23,6 +23,7 @@
 #include <stdint.h>
 #include <inttypes.h>
 #include <string.h>
+#include <errno.h>

 #include <libnbd.h>

@@ -30,14 +31,56 @@
 #define XSTR(s) #s
 #define STR(s) XSTR(s)

+static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512];
+static const char *progname;
+
+static int
+pread_cb (void *data, const void *buf, size_t count, uint64_t offset,
+          int error, int status)
+{
+  int *calls = data;
+  ++*calls;
+
+  if (buf != rbuf || count != sizeof rbuf) {
+    fprintf (stderr, "%s: callback called with wrong buffer\n",
progname);
+    exit (EXIT_FAILURE);
+  }
+  if (offset != 2 * sizeof rbuf) {
+    fprintf (stderr, "%s: callback called with wrong offset\n",
progname);
+    exit (EXIT_FAILURE);
+  }
+  if (error != 0) {
+    fprintf (stderr, "%s: callback called with unexpected error\n",
progname);
+    exit (EXIT_FAILURE);
+  }
+  if (status != LIBNBD_READ_DATA) {
+    fprintf (stderr, "%s: callback called with wrong status\n",
progname);
+    exit (EXIT_FAILURE);
+  }
+
+  if (memcmp (rbuf, wbuf, sizeof rbuf) != 0) {
+    fprintf (stderr, "%s: DATA INTEGRITY ERROR!\n", progname);
+    exit (EXIT_FAILURE);
+  }
+
+  if (*calls > 1) {
+    errno = EPROTO; /* Something NBD servers can't send */
+    return -1;
+  }
+
+  return 0;
+}
+
 int
 main (int argc, char *argv[])
 {
   struct nbd_handle *nbd;
-  char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512];
   int64_t r;
   char *args[] = { "nbdkit", "-s", "-o",
"--exit-with-parent", "-v",
                    "memory", "size=" STR(SIZE), NULL };
+  int calls = 0;
+
+  progname = argv[0];

   nbd = nbd_create ();
   if (nbd == NULL) {
@@ -61,12 +104,13 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

-  if (nbd_pwrite (nbd, wbuf, sizeof wbuf, 0, 0) == -1) {
+  /* Plain I/O */
+  if (nbd_pwrite (nbd, wbuf, sizeof wbuf, 2 * sizeof wbuf, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }

-  if (nbd_pread (nbd, rbuf, sizeof rbuf, 0, 0) == -1) {
+  if (nbd_pread (nbd, rbuf, sizeof rbuf, 2 * sizeof rbuf, 0) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
   }
@@ -76,6 +120,34 @@ main (int argc, char *argv[])
     exit (EXIT_FAILURE);
   }

+  /* Test again for callback operation. */
+  memset (rbuf, 0, sizeof rbuf);
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2 * sizeof rbuf,
+                            &calls, pread_cb, 0) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+
+  if (calls != 1) {
+    fprintf (stderr, "%s: callback called wrong number of times\n",
argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  if (memcmp (rbuf, wbuf, sizeof rbuf) != 0) {
+    fprintf (stderr, "%s: DATA INTEGRITY ERROR!\n", argv[0]);
+    exit (EXIT_FAILURE);
+  }
+
+  /* Also test that callback errors are reflected correctly. */
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2 * sizeof rbuf,
+                            &calls, pread_cb, 0) != -1) {
+    fprintf (stderr, "%s: expected failure from callback\n",
argv[0]);
+    exit (EXIT_FAILURE);
+  }
+  if (nbd_get_errno () != EPROTO) {
+    fprintf (stderr, "%s: wrong errno value after failed callback\n",
argv[0]);
+    exit (EXIT_FAILURE);
+  }
+
   if (nbd_shutdown (nbd) == -1) {
     fprintf (stderr, "%s\n", nbd_get_error ());
     exit (EXIT_FAILURE);
-- 
2.20.1

When structured replies are negotiated, the server may advertise
support for the DF flag (the server promises to return at most one
data/hole chunk, or to fail with NBD_EOVERFLOW if the chunk would be
too large). As both nbdkit and qemu-nbd support this flag (the former
only trivially, but the latter by not compressing holes over the
wire), it is worth exposing to clients, if only for testing purposes.

I chose to specifically reject the flag for plain nbd_[aio_]pread
(about all it can do is cause a read to fail that would otherwise
succeed) and only accept it for nbd_[aio_]pread_structured, but it
would be easy enough to change the one to forward to the other if we
wanted.
---
 generator/generator              | 22 +++++++++++++++++++---
 interop/structured-read.c        | 31 ++++++++++++++++++++++++-------
 lib/flags.c                      | 11 +++++++++++
 lib/nbd-protocol.h               |  1 +
 lib/protocol.c                   |  1 +
 lib/rw.c                         | 14 ++++++++++++--
 python/t/405-pread-structured.py |  6 ++++++
 7 files changed, 74 insertions(+), 12 deletions(-)

diff --git a/generator/generator b/generator/generator
index cf9876f..da60486 100755
--- a/generator/generator
+++ b/generator/generator
@@ -1243,6 +1243,17 @@ Returns true if the server supports the zero command
 the server does not.";
   };

+  "can_df", {
+    default_call with
+    args = []; ret = RBool;
+    shortdesc = "does the server support the don't fragment flag to
pread?";
+    longdesc = "\
+Returns true if the server supports structured reads with an
+ability to request a non-fragmented read (see C<nbd_pread_structured>,
+C<nbd_aio_pread_structured>).  Returns false if the server either lacks
+structured reads or if it does not support a non-fragmented read
request.";
+  };
+
   "can_multi_conn", {
     default_call with
     args = []; ret = RBool;
@@ -1306,7 +1317,8 @@ at C<offset> and ending at C<offset> +
C<count> - 1.  NBD
 can only read all or nothing using this call.  The call
 returns when the data has been read fully into C<buf> or there is an
 error.  See also C<nbd_pread_structured>, if finer visibility is
-required into the server's replies.
+required into the server's replies, or if you want to use
+C<LIBNBD_CMD_FLAG_DF>.

 The C<flags> parameter must be C<0> for now (it exists for future
NBD
 protocol extensions).";
@@ -1379,8 +1391,11 @@ the server obeyed the requirement that a read call must
not have
 overlapping chunks and must not succeed without enough chunks to cover
 the entire request.

-The C<flags> parameter must be C<0> for now (it exists for future
NBD
-protocol extensions).";
+The C<flags> parameter may be C<0> for no flags, or may contain
+C<LIBNBD_CMD_FLAG_DF> meaning that the server should not reply with
+more than one fragment (if that is supported - some servers cannot do
+this, see C<nbd_can_df>). Libnbd does not validate that the server
+actually obeys the flag.";
   };

   "pwrite", {
@@ -2025,6 +2040,7 @@ let constants = [

   "CMD_FLAG_FUA",        1 lsl 0;
   "CMD_FLAG_NO_HOLE",    1 lsl 1;
+  "CMD_FLAG_DF",         1 lsl 2;
   "CMD_FLAG_REQ_ONE",    1 lsl 3;

   "READ_DATA",           1;
diff --git a/interop/structured-read.c b/interop/structured-read.c
index ea47254..cf8b893 100644
--- a/interop/structured-read.c
+++ b/interop/structured-read.c
@@ -38,7 +38,7 @@ static const char *unixsocket;
 static char rbuf[1024];

 struct data {
-  //XXX  bool df;    /* input: true if DF flag was passed to request */
+  bool df;         /* input: true if DF flag was passed to request */
   int count;       /* input: count of expected remaining calls */
   bool fail;       /* input: true to return failure */
   bool seen_hole;  /* output: true if hole encountered */
@@ -60,15 +60,24 @@ read_cb (void *opaque, const void *bufv, size_t count,
uint64_t offset,

   switch (status) {
   case LIBNBD_READ_DATA:
-    // XXX if (df...)
-    assert (buf == rbuf + 512);
-    assert (count == 512);
-    assert (offset == 2048 + 512);
-    assert (buf[0] == 1 && memcmp (buf, buf + 1, 511) == 0);
+    if (data->df) {
+      assert (buf == rbuf);
+      assert (count == 1024);
+      assert (offset == 2048);
+      assert (buf[0] == 0 && memcmp (buf, buf + 1, 511) == 0);
+      assert (buf[512] == 1 && memcmp (buf + 512, buf + 513, 511) ==
0);
+    }
+    else {
+      assert (buf == rbuf + 512);
+      assert (count == 512);
+      assert (offset == 2048 + 512);
+      assert (buf[0] == 1 && memcmp (buf, buf + 1, 511) == 0);
+    }
     assert (!data->seen_data);
     data->seen_data = true;
     break;
   case LIBNBD_READ_HOLE:
+    assert (!data->df); /* Relies on qemu-nbd's behavior */
     assert (buf == rbuf);
     assert (count == 512);
     assert (offset == 2048);
@@ -134,7 +143,15 @@ main (int argc, char *argv[])
   }
   assert (data.seen_data && data.seen_hole);

-  // XXX Repeat with DF flag
+  /* Repeat with DF flag. */
+  memset (rbuf, 2, sizeof rbuf);
+  data = (struct data) { .df = true, .count = 1, };
+  if (nbd_pread_structured (nbd, rbuf, sizeof rbuf, 2048, &data, read_cb,
+                            LIBNBD_CMD_FLAG_DF) == -1) {
+    fprintf (stderr, "%s\n", nbd_get_error ());
+    exit (EXIT_FAILURE);
+  }
+  assert (data.seen_data && !data.seen_hole);

   /* Trigger a failed callback, to prove connection stays up. With
    * reads, all chunks trigger a callback even after failure, but the
diff --git a/lib/flags.c b/lib/flags.c
index 421a7d2..cdbc28f 100644
--- a/lib/flags.c
+++ b/lib/flags.c
@@ -42,6 +42,11 @@ nbd_internal_set_size_and_flags (struct nbd_handle *h,
     return -1;
   }

+  if (eflags & NBD_FLAG_SEND_DF && !h->structured_replies) {
+    debug (h, "server lacks structured replies, ignoring claim of
df");
+    eflags &= ~NBD_FLAG_SEND_DF;
+  }
+
   h->exportsize = exportsize;
   h->eflags = eflags;
   return 0;
@@ -95,6 +100,12 @@ nbd_unlocked_can_zero (struct nbd_handle *h)
   return get_flag (h, NBD_FLAG_SEND_WRITE_ZEROES);
 }

+int
+nbd_unlocked_can_df (struct nbd_handle *h)
+{
+  return get_flag (h, NBD_FLAG_SEND_DF);
+}
+
 int
 nbd_unlocked_can_multi_conn (struct nbd_handle *h)
 {
diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h
index 071971e..405af3e 100644
--- a/lib/nbd-protocol.h
+++ b/lib/nbd-protocol.h
@@ -245,6 +245,7 @@ struct nbd_structured_reply_error {
 #define NBD_ENOMEM     12
 #define NBD_EINVAL     22
 #define NBD_ENOSPC     28
+#define NBD_EOVERFLOW  75
 #define NBD_ESHUTDOWN 108

 #endif /* NBD_PROTOCOL_H */
diff --git a/lib/protocol.c b/lib/protocol.c
index d3ac0b4..6087887 100644
--- a/lib/protocol.c
+++ b/lib/protocol.c
@@ -35,6 +35,7 @@ nbd_internal_errno_of_nbd_error (uint32_t error)
   case NBD_ENOMEM: return ENOMEM;
   case NBD_EINVAL: return EINVAL;
   case NBD_ENOSPC: return ENOSPC;
+  case NBD_EOVERFLOW: return EOVERFLOW;
   case NBD_ESHUTDOWN: return ESHUTDOWN;
   default: return EINVAL;
   }
diff --git a/lib/rw.c b/lib/rw.c
index 24dbc4e..2dc60de 100644
--- a/lib/rw.c
+++ b/lib/rw.c
@@ -238,6 +238,10 @@ int64_t
 nbd_unlocked_aio_pread (struct nbd_handle *h, void *buf,
                         size_t count, uint64_t offset, uint32_t flags)
 {
+  /* We could silently accept flag DF, but it really only makes sense
+   * with callbacks, because otherwise there is no observable change
+   * except that the server may fail where it would otherwise succeed.
+   */
   if (flags != 0) {
     set_error (EINVAL, "invalid flag: %" PRIu32, flags);
     return -1;
@@ -254,12 +258,18 @@ nbd_unlocked_aio_pread_structured (struct nbd_handle *h,
void *buf,
 {
   struct command_cb cb = { .opaque = opaque, .fn.read = read, };

-  if (flags != 0) {
+  if ((flags & ~LIBNBD_CMD_FLAG_DF) != 0) {
     set_error (EINVAL, "invalid flag: %" PRIu32, flags);
     return -1;
   }

-  return nbd_internal_command_common (h, 0, NBD_CMD_READ, offset, count,
+  if ((flags & LIBNBD_CMD_FLAG_DF) != 0 &&
+      nbd_unlocked_can_df (h) != 1) {
+    set_error (EINVAL, "server does not support the DF flag");
+    return -1;
+  }
+
+  return nbd_internal_command_common (h, flags, NBD_CMD_READ, offset, count,
                                       buf, &cb);
 }

diff --git a/python/t/405-pread-structured.py b/python/t/405-pread-structured.py
index 51f0144..1bfa162 100644
--- a/python/t/405-pread-structured.py
+++ b/python/t/405-pread-structured.py
@@ -35,3 +35,9 @@ buf = h.pread_structured (512, 0, 42, f)
 print ("%r" % buf)

 assert buf == expected
+
+buf = h.pread_structured (512, 0, 42, f, nbd.CMD_FLAG_DF)
+
+print ("%r" % buf)
+
+assert buf == expected
-- 
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
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Thu, Jun 20, 2019 at 08:45:05PM -0500, Eric Blake
wrote:
> If any callback fails, and if no prior error was set, then the
> callback's failure becomes the failure reason for the overall
> read. (Note that this is different from the block status callback,
> which for now quits using the callback on subsequent chunks if an
> earlier chunk failed - we may decide to change one or the other for
> consistency before the API is stable.)
> 
> Nothing actually passes a callback function yet, so for now this is no
> functional change; but this will make it possible for the next patch
> to add an 'nbd_aio_pread_structred' API.
I don't think non-C callbacks will be able to reliably set errno, but
that's something we can solve in future.  Therefore ACK.

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

Seems to address everything from the previous review, so:

ACK

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

ACK 4 and 5, therefore ACK series.

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