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

[Vladimir Sementsov-Ogievskiy]

23.03.2019 17:24, 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
> 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) {

Hmm, I don't see, what is changed. You just don't set ret and err if there 
already
set request_ret.. So, finally it may lead to other return code in 
nbd_client_co_block_status
and local_err will not be set and traced. But there is no connection close in 
this case
in current code, s->quit is not set.

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


-- 
Best regards,
Vladimir