thr3ads.net - Libguestfs - Re: [Libguestfs] nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected [Mar 2019]

If this information is useful, please help other people find it:
Share via:

Richard W.M. Jones

2019-Mar-23 11:42 UTC

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

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.

Unfortunately RHEL 7.6 & 7.7 (qemu-kvm-rhev-2.12.0-25.el7) doesn't
contain the fix.  I wonder if we should:

(a) Not worry about it - it's Red Hat's problem.

(b) Try and get it fixed in RHEL.  I filed a BZ already but I guess it
won't be fixed any time soon: https://bugzilla.redhat.com/1692018

(c) Add a workaround in nbdkit.

I feel that we probably shouldn't be broken out of the box with RHEL
and CentOS 7.6 and 7.7, even though it's not an nbdkit (or qemu) bug.

Rich.

----------------------------------------------------------------------
89aa0d87634e2cb98517509dc8bdb876f26ecf8b is the first bad commit
commit 89aa0d87634e2cb98517509dc8bdb876f26ecf8b
Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
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.
    
    To solve it, just initialize received to false. Initialize received_id
    too, just to make all possible checkers happy.
    
    Bug was introduced in 78a33ab58782efdb206de14 "nbd: BLOCK_STATUS for
    standard get_block_status function: client part" with the whole
    function.
    
    Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
    Message-Id: <20180427142002.21930-2-vsementsov@virtuozzo.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>
    CC: qemu-stable@nongnu.org
    Signed-off-by: Eric Blake <eblake@redhat.com>

:040000 040000 9993feb118af1a9a59dbc8fe92015a324e93e557
14db90d621d6b7f1ee5ff97a0ca2cb92f6f2f7e9 M	nbd


-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v

Eric Blake

2019-Mar-23 11:57 UTC

head link

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

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.
> 
> Unfortunately RHEL 7.6 & 7.7 (qemu-kvm-rhev-2.12.0-25.el7) doesn't
> contain the fix.  I wonder if we should:
> 
> (a) Not worry about it - it's Red Hat's problem.
Upstream qemu has moved on, so from that perspective, yes, it's Red
Hat's problem (Red Hat must ensure that any versions of qemu and nbdkit
that it ships and which must interoperate do so correctly, whether that
be qemu from RHEL 7 and nbdkit from RHEL 8, or the other way around).
> 
> (b) Try and get it fixed in RHEL.  I filed a BZ already but I guess it
> won't be fixed any time soon: https://bugzilla.redhat.com/1692018
Several RHEL 7 bugs against NBD are pending - there's hope for RHEL 7.7,
but you are also right that guessing timelines is hard.
> 
> (c) Add a workaround in nbdkit.
> 
And what workaround would that be? Looking at the qemu patch, the
problem is that qemu asked for "base:allocation" and nbdkit replied
with
0 contexts. The workaround would either be to reply with
NBD_REP_ERR_UNSUP (the behavior before NBD_OPT_SET_META_CONTEXT was
added), or to implement "base:allocation" (even if we implement the
poor-man's version that always reports that the entire image is data,
for all requests, without any feedback from the plugins).  The former
feels odd, but the latter seems oddly appealing as progress towards our
end goal (kind of how our initial implementation of NBD_CMD_WRITE_ZEROES
was always advertised even before we finished wiring up plug support).
> I feel that we probably shouldn't be broken out of the box with RHEL
> and CentOS 7.6 and 7.7, even though it's not an nbdkit (or qemu) bug.
Correct that it is not nbdkit's bug; it was a bug in qemu (but one that
upstream no longer worries about), and old enough of a qemu release that
I don't the the NBD protocol needs to bother mentioning it as a special
case.
> 
> Rich.
> 
> ----------------------------------------------------------------------
> 89aa0d87634e2cb98517509dc8bdb876f26ecf8b is the first bad commit
> commit 89aa0d87634e2cb98517509dc8bdb876f26ecf8b
> Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 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.
>     
>     To solve it, just initialize received to false. Initialize received_id
>     too, just to make all possible checkers happy.
>     
>     Bug was introduced in 78a33ab58782efdb206de14 "nbd: BLOCK_STATUS
for
>     standard get_block_status function: client part" with the whole
>     function.
>     
>     Signed-off-by: Vladimir Sementsov-Ogievskiy
<vsementsov@virtuozzo.com>
>     Message-Id: <20180427142002.21930-2-vsementsov@virtuozzo.com>
>     Reviewed-by: Eric Blake <eblake@redhat.com>
>     CC: qemu-stable@nongnu.org
>     Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> :040000 040000 9993feb118af1a9a59dbc8fe92015a324e93e557
14db90d621d6b7f1ee5ff97a0ca2cb92f6f2f7e9 M	nbd
> 
> 
-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Richard W.M. Jones

2019-Mar-23 12:21 UTC

head link

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

On Sat, Mar 23, 2019 at 06:57:14AM -0500, Eric Blake
wrote:> On 3/23/19 6:42 AM, Richard W.M. Jones wrote:
> > (b) Try and get it fixed in RHEL.  I filed a BZ already but I guess it
> > won't be fixed any time soon: https://bugzilla.redhat.com/1692018
> 
> Several RHEL 7 bugs against NBD are pending - there's hope for RHEL
7.7,
> but you are also right that guessing timelines is hard.
Yes, my worry is that if we miss 7.7 then this bug will exist forever
since as far as I know we (Red Hat) are not planning any further qemu
improvements in 7.8 and above.
> > (c) Add a workaround in nbdkit.
> > 
> 
> And what workaround would that be? Looking at the qemu patch, the
> problem is that qemu asked for "base:allocation" and nbdkit
replied with
> 0 contexts. The workaround would either be to reply with
> NBD_REP_ERR_UNSUP (the behavior before NBD_OPT_SET_META_CONTEXT was
> added), or to implement "base:allocation" (even if we implement
the
> poor-man's version that always reports that the entire image is data,
> for all requests, without any feedback from the plugins).  The former
> feels odd, but the latter seems oddly appealing as progress towards our
> end goal (kind of how our initial implementation of NBD_CMD_WRITE_ZEROES
> was always advertised even before we finished wiring up plug support).
A good point here is what happens with the block-status branch[1].
Let's see:

$ ./nbdkit memory size=64M --run '/home/rjones/d/qemu/qemu-img convert $nbd
/var/tmp/out'
qemu-img: Payload too large
nbdkit: memory.1: error: write reply: NBD_CMD_BLOCK_STATUS: Broken pipe

Oh dear.  I believe this is actually a bug in the block status code so
let's see if I can narrow this down first ...

Rich.

[1] https://github.com/rwmjones/nbdkit/tree/block-status

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

Eric Blake

2019-Mar-23 12:26 UTC

head link

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

[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 <vsementsov@virtuozzo.com>
>> 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
...
17218@1553343857.048440:nbd_send_request Sending request to server: {
.from = 0, .len = 1049088, .handle = 94796979915216, .flags = 0x8, .type
= 7 (block status) }
17218@1553343857.048643:nbd_receive_simple_reply Got simple reply: {
.error = 22 (EINVAL), handle = 94796979915216 }
17218@1553343857.048655: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

Richard W.M. Jones

2019-Mar-25 18:07 UTC

head link

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

[Removed public mailing list]

On Mon, Mar 25, 2019 at 01:45:32PM -0400, John Snow
wrote:> 
> 
> On 3/23/19 8:21 AM, Richard W.M. Jones wrote:
> > On Sat, Mar 23, 2019 at 06:57:14AM -0500, Eric Blake wrote:
> >> On 3/23/19 6:42 AM, Richard W.M. Jones wrote:
> >>> (b) Try and get it fixed in RHEL.  I filed a BZ already but I
guess it
> >>> won't be fixed any time soon:
https://bugzilla.redhat.com/1692018
> >>
> >> Several RHEL 7 bugs against NBD are pending - there's hope for
RHEL 7.7,
> >> but you are also right that guessing timelines is hard.
> > 
> > Yes, my worry is that if we miss 7.7 then this bug will exist forever
> > since as far as I know we (Red Hat) are not planning any further qemu
> > improvements in 7.8 and above.
> > 
> 
> Feel free to add some ACKs to the lengthy backports I've posted in 7.7
> recently ...
> 
> 3.1.0 NBD Roundup is probably particularly of interest to you right now.
Is this on rhvirt-patches?  I can't see it ...

Rich.
> I'm working on 4.0.0 NBD roundup right now, too.
-- 
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

Reasonably Related Threads

Search for more apparently analagous threads

Libguestfs - Mar 2019 - Re: nbdkit & qemu 2.12: qemu-img: Protocol error: simple reply when structured reply chunk was expected

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

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

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

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

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

Reasonably Related Threads