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

[Richard W.M. Jones]

On Sat, Mar 23, 2019 at 09:24:55AM -0500, Eric Blake wrote:
> 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

"However"

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

This latter part fixes the silent qemu client disconnection that
happens when the server sends back a large number of extents in a
single block status reply?

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

OK, got it.  However I think I'd prefer to see how it goes fixing qemu
in RHEL 7 first.

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

ACK

Rich.

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