[Qemu-devel] [PATCH for-4.0] nbd: Permit simple error to NBD_CMD_BLOCK_STATUS

[Eric Blake]

The NBD spec is clear that when structured replies are active, a
simple error reply is acceptable to any command except for
NBD_CMD_READ.  However, we were mistakenly requiring structured errors
for NBD_CMD_BLOCK_STATUS, and hanging up on a server that gave a
simple error (since qemu does not behave as such a server, we didn't
notice the problem until now).  Broken since its introduction in
commit 78a33ab5 (v2.12).

Howeve, even if we had gotten it correct to accept simple errors back
then, we run into another problem: the client treats the server's
reply as fatal and hangs up on the connection, instead of merely
failing the block status request but being ready for the next
command. Broken in commit 7f86068d (unreleased).

Signed-off-by: Eric Blake <address@hidden>
---

I confirmed that when backporting things for qemu 2.12 through 3.1,
the first hunk is sufficient to let clients tolerate a simple error
without hanging up (the second hunk is only required for the 4.0 code
base).

Rich - if you choose to make nbdkit work around the qemu 2.12 bug
where it refuses to communicate with a server that supports
NBD_OPT_SET_META_CONTEXT but not base:allocation, you'll also want to
make sure that you send a structured error instead of a simple error
to any failures of NBD_CMD_BLOCK_STATUS.

 block/nbd-client.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index bfbaf7ebe94..5fc9ea40383 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -718,9 +718,7 @@ static int 
nbd_co_receive_blockstatus_reply(NBDClientSession *s,
     bool received = false;

     assert(!extent->length);
-    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, s->info.structured_reply,
-                            NULL, &reply, &payload)
-    {
+    NBD_FOREACH_REPLY_CHUNK(s, iter, handle, false, NULL, &reply, &payload) {
         int ret;
         NBDStructuredReplyChunk *chunk = &reply.structured;

@@ -758,9 +756,11 @@ static int 
nbd_co_receive_blockstatus_reply(NBDClientSession *s,
         payload = NULL;
     }

-    if (!extent->length && !iter.err) {
-        error_setg(&iter.err,
-                   "Server did not reply with any status extents");
+    if (!extent->length && !iter.request_ret) {
+        if (!iter.err) {
+            error_setg(&iter.err,
+                       "Server did not reply with any status extents");
+        }
         if (!iter.ret) {
             iter.ret = -EIO;
         }
-- 
2.20.1