Re: [Qemu-devel] nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected

[Eric Blake]

[adding qemu]

On 3/23/19 6:57 AM, Eric Blake wrote:
> On 3/23/19 6:42 AM, Richard W.M. Jones wrote:
>> nbdkit (upstream 5a7a394c699) currently fails with qemu 2.12.0:
>>
>>   $ ./nbdkit memory size=64M --run 'qemu-img convert $nbd /var/tmp/out'
>>   nbdkit: memory.2: error: invalid request: unknown command (7) ignored
>>   qemu-img: Protocol error: simple reply when structured reply chunk was 
>> expected
>>
>> This was a bug in qemu which was fixed upstream quite a long time ago
>> by the commit I've attached at the end of this email.
>>

>> ----------------------------------------------------------------------
>> 89aa0d87634e2cb98517509dc8bdb876f26ecf8b is the first bad commit
>> commit 89aa0d87634e2cb98517509dc8bdb876f26ecf8b
>> Author: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Date:   Fri Apr 27 17:20:01 2018 +0300
>>
>>     nbd/client: fix nbd_negotiate_simple_meta_context
>>     
>>     Initialize received variable. Otherwise, is is possible for server to
>>     answer without any contexts, but we will set context_id to something
>>     random (received_id is not initialized too) and return 1, which is
>>     wrong.
>>     

Ouch - upstream qemu still has a latent bug in this area. When qemu as
server sends an error to NBD_CMD_BLOCK_STATUS, it uses a structured
error reply, so we haven't tickled it before.  But the NBD spec permits
a simple error reply to any command except NBD_CMD_READ when structured
replies are negotiated, and that's what nbdkit currently implements. If
I add this hack patch to qemu's server (to intentionally fail
NBD_CMD_BLOCK_STATUS with a simple error), then the qemu client _should_
be able to just report that block status failed and keep the connection
alive, rather than its current behavior of hanging up. So we still need
a patch for qemu 4.0 for the client to accept simple errors. :(

With the hack applied, I got:

$ ./qemu-io -f raw nbd://localhost:10809 --trace=nbd_\* -c map
...
address@hidden:nbd_send_request Sending request to server: {
.from = 0, .len = 1049088, .handle = 94796979915216, .flags = 0x8, .type
= 7 (block status) }
address@hidden:nbd_receive_simple_reply Got simple reply: {
.error = 22 (EINVAL), handle = 94796979915216 }
address@hidden:nbd_co_request_fail Request failed { .from = 0,
.len = 1049088, .handle = 94796979915216, .flags = 0x8, .type = 7 (block
status) } ret = -22, err: Protocol error: simple reply when structured
reply chunk was expected
Failed to get allocation status: Invalid argument


diff --git i/nbd/server.c w/nbd/server.c
index fd013a2817a..35f549abbf9 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -2148,16 +2148,16 @@ static int nbd_co_receive_request(NBDRequestData
*req, NBDRequest *request,
  * Returns 0 if connection is still live, -errno on failure to talk to
client
  */
 static coroutine_fn int nbd_send_generic_reply(NBDClient *client,
-                                               uint64_t handle,
+                                               NBDRequest *request,
                                                int ret,
                                                const char *error_msg,
                                                Error **errp)
 {
-    if (client->structured_reply && ret < 0) {
-        return nbd_co_send_structured_error(client, handle, -ret,
error_msg,
+    if (client->structured_reply && ret < 0 && request->type ==
NBD_CMD_READ) {
+        return nbd_co_send_structured_error(client, request->handle,
-ret, error_msg,
                                             errp);
     } else {
-        return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0,
+        return nbd_co_send_simple_reply(client, request->handle, ret <
0 ? -ret : 0,
                                         NULL, 0, errp);
     }
 }
@@ -2177,7 +2177,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient
*client, NBDRequest *request,
     if (request->flags & NBD_CMD_FLAG_FUA) {
         ret = blk_co_flush(exp->blk);
         if (ret < 0) {
-            return nbd_send_generic_reply(client, request->handle, ret,
+            return nbd_send_generic_reply(client, request, ret,
                                           "flush failed", errp);
         }
     }
@@ -2192,7 +2192,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient
*client, NBDRequest *request,
     ret = blk_pread(exp->blk, request->from + exp->dev_offset, data,
                     request->len);
     if (ret < 0 || request->type == NBD_CMD_CACHE) {
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "reading from file failed", errp);
     }

@@ -2234,7 +2234,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
         }
         ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
                          data, request->len, flags);
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "writing to file failed", errp);

     case NBD_CMD_WRITE_ZEROES:
@@ -2247,7 +2247,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
         }
         ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
                                 request->len, flags);
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "writing to file failed", errp);

     case NBD_CMD_DISC:
@@ -2256,7 +2256,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,

     case NBD_CMD_FLUSH:
         ret = blk_co_flush(exp->blk);
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "flush failed", errp);

     case NBD_CMD_TRIM:
@@ -2265,12 +2265,14 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
         if (ret == 0 && request->flags & NBD_CMD_FLAG_FUA) {
             ret = blk_co_flush(exp->blk);
         }
-        return nbd_send_generic_reply(client, request->handle, ret,
+        return nbd_send_generic_reply(client, request, ret,
                                       "discard failed", errp);

     case NBD_CMD_BLOCK_STATUS:
+        return nbd_send_generic_reply(client, request, -EINVAL,
+                                      "no status for you today", errp);
         if (!request->len) {
-            return nbd_send_generic_reply(client, request->handle, -EINVAL,
+            return nbd_send_generic_reply(client, request, -EINVAL,
                                           "need non-zero length", errp);
         }
         if (client->export_meta.valid &&
@@ -2304,7 +2306,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,

             return ret;
         } else {
-            return nbd_send_generic_reply(client, request->handle, -EINVAL,
+            return nbd_send_generic_reply(client, request, -EINVAL,
                                           "CMD_BLOCK_STATUS not
negotiated",
                                           errp);
         }
@@ -2312,7 +2314,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
     default:
         msg = g_strdup_printf("invalid request type (%" PRIu32 ")
received",
                               request->type);
-        ret = nbd_send_generic_reply(client, request->handle, -EINVAL, msg,
+        ret = nbd_send_generic_reply(client, request, -EINVAL, msg,
                                      errp);
         g_free(msg);
         return ret;
@@ -2357,7 +2359,7 @@ static coroutine_fn void nbd_trip(void *opaque)
         Error *export_err = local_err;

         local_err = NULL;
-        ret = nbd_send_generic_reply(client, request.handle, -EINVAL,
+        ret = nbd_send_generic_reply(client, &request, -EINVAL,
                                      error_get_pretty(export_err),
&local_err);
         error_free(export_err);
     } else {


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature