Re: [Qemu-block] [PATCH for-4.0 v2 1/2] nbd: Don't lose server's error to NBD_CMD_BLOCK_STATUS

[Vladimir Sementsov-Ogievskiy]

25.03.2019 22:01, Eric Blake wrote:
> When the server replies with a (structured [*]) error to
> NBD_CMD_BLOCK_STATUS, without any extent information sent first, the
> client code was blindly throwing away the server's error code and
> instead telling the caller that EIO occurred.  This has been broken
> since its introduction in 78a33ab5 (v2.12, where we should have called:
>     error_setg(&local_err, "Server did not reply with any status extents");
>     nbd_iter_error(&iter, false, -EIO, &local_err);
> to declare the situation as a non-fatal error if no earlier error had
> already been flagged, rather than just blindly slamming iter.err and
> iter.ret), although it is more noticeable since commit 7f86068d, which
> actually tries hard to preserve the server's code thanks to a separate
> iter.request_ret.
> 
> [*] The spec is clear that the server is also permitted to reply with
> a simple error, but that's a separate fix.
> 
> I was able to provoke this scenario with a hack to the server, then
> seeing whether ENOMEM makes it back to the caller:
> 
> | diff --git a/nbd/server.c b/nbd/server.c
> | index fd013a2817a..29c7995de02 100644
> | --- a/nbd/server.c
> | +++ b/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_send_generic_reply(client, request->handle, -ENOMEM,
> | +                                      "no status for you today", 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>
> ---
>   block/nbd-client.c | 9 +++------
>   1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 8fe660b6093..b37a5963013 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -769,12 +769,9 @@ 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 (!iter.ret) {
> -            iter.ret = -EIO;
> -        }
> +    if (!extent->length && !iter.request_ret) {
> +        error_setg(&local_err, "Server did not reply with any status 
> extents");
> +        nbd_iter_channel_error(&iter, -EIO, &local_err);
>       }

So, replying with error and not sending any extents is not violation of the 
protocol and
not a reason for channel_error, agreed. Thank you for describing and splitting 
this.

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



-- 
Best regards,
Vladimir