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

[Vladimir Sementsov-Ogievskiy]

25.03.2019 22:01, 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).
> 
> Noticed while debugging a separate failure reported by nbdkit while
> working out its initial implementation of BLOCK_STATUS, although it
> turns out that nbdkit also chose to send structured error replies for
> BLOCK_STATUS, so I had to manually provoke the situation by hacking
> qemu's server to send a simple error reply:
> 
> | diff --git i/nbd/server.c w/nbd/server.c
> | index fd013a2817a..833288d7c45 100644
> | 00--- i/nbd/server.c
> | +++ w/nbd/server.c
> | @@ -2269,6 +2269,8 @@ static coroutine_fn int nbd_handle_request(NBDClient 
> *client,
> |                                        "discard failed", errp);
> |
> |      case NBD_CMD_BLOCK_STATUS:
> | +        return nbd_co_send_simple_reply(client, request->handle, ENOMEM,
> | +                                        NULL, 0, errp);
> |          if (!request->len) {
> |              return nbd_send_generic_reply(client, request->handle, -EINVAL,
> |                                            "need non-zero length", errp);
> |
> 
> Signed-off-by: Eric Blake <address@hidden>
> Message-Id: <address@hidden>
> Acked-by: Richard W.M. Jones <address@hidden>


Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> ---
>   block/nbd-client.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index b37a5963013..a3b70d14004 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -729,9 +729,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;
> 


-- 
Best regards,
Vladimir