[Libguestfs] [libnbd PATCH 3/3] states: Tolerate simple reply to structured read request

[Eric Blake]

Another patch in the series of being more tolerant to various server
bugs.  Although a server is non-compliant for using a simple reply to
NBD_CMD_READ once structured commands are negotiated, it is just as
easy to read the packet and continue on after guaranteeing we report
an error.

In the process, I noticed that in the corner case of a server that
starts with a structured packet and follows with a simple reply, we
can no longer assume that the simple reply is the only receipt of
data, so we have to be more careful with assertions and increments to
data_seen.

As in earlier patches, the following temporary one-line tweak to
nbdkit will produce a server that demonstrates the scenario:

| diff --git i/server/protocol.c w/server/protocol.c
| index 2ac77055..ac359ca1 100644
| --- i/server/protocol.c
| +++ w/server/protocol.c
| @@ -738,7 +738,7 @@ protocol_recv_request_send_reply (void)
|     * us from sending human-readable error messages to the client, so
|     * we should reconsider this in future.
|     */
| -  if (conn->structured_replies &&
| +  if (conn->structured_replies && !error &&
|        (cmd == NBD_CMD_READ || cmd == NBD_CMD_BLOCK_STATUS)) {
|      if (!error) {
|        if (cmd == NBD_CMD_READ)

The following command line:

$ ./run /path/to/nbdkit -U - memory 1M --filter=error error-pread-rate=100% \
    --run 'nbdsh -u "$uri" -c "\
try:
  h.pread(1,0)
except nbd.Error as ex:
  print(ex.string)
h.flush()
print(h.get_size())
"'

demonstrates the following issue pre-patch:

nbdkit: memory.0: error: injecting EIO error into pread
nbd_pread: server sent unexpected simple reply for read
nbdsh: command line script failed: nbd_flush: invalid state: DEAD: the handle must be connected with the server: Invalid argument

and the nicer flow post-patch:

nbdkit: memory.0: error: injecting EIO error into pread
nbd_pread: read: command failed: Protocol error
1048576

where we report EPROTO rather than the server's EIO.
---
 generator/states-reply-simple.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c
index 24f7efa..ac58823 100644
--- a/generator/states-reply-simple.c
+++ b/generator/states-reply-simple.c
@@ -49,17 +49,23 @@ STATE_MACHINE {
     return 0;
   }

+  /* Although a server with structured replies negotiated is in error
+   * for using a simple reply to NBD_CMD_READ, we can cope with the
+   * packet, but diagnose it by failing the read with EPROTO.
+   */
   if (cmd->type == NBD_CMD_READ && h->structured_replies) {
-    set_error (0, "server sent unexpected simple reply for read");
-    SET_NEXT_STATE(%.DEAD);
-    return 0;
+    debug (h, "server sent unexpected simple reply for read");
+    if (cmd->error == 0)
+      cmd->error = EPROTO;
   }

-  cmd->error = nbd_internal_errno_of_nbd_error (error);
-  if (cmd->error == 0 && cmd->type == NBD_CMD_READ) {
+  error = nbd_internal_errno_of_nbd_error (error);
+  if (cmd->error == 0)
+    cmd->error = error;
+  if (error == 0 && cmd->type == NBD_CMD_READ) {
     h->rbuf = cmd->data;
     h->rlen = cmd->count;
-    cmd->data_seen = cmd->count;
+    cmd->data_seen += cmd->count;
     SET_NEXT_STATE (%RECV_READ_PAYLOAD);
   }
   else {
@@ -80,9 +86,8 @@ STATE_MACHINE {
     /* guaranteed by START */
     assert (cmd);
     if (CALLBACK_IS_NOT_NULL (cmd->cb.fn.chunk)) {
-      int error = 0;
+      int error = cmd->error;

-      assert (cmd->error == 0);
       if (CALL_CALLBACK (cmd->cb.fn.chunk,
                          cmd->data, cmd->count,
                          cmd->offset, LIBNBD_READ_DATA,
-- 
2.37.1