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 <ebl...@redhat.com>
> Message-Id: <20190323142455.5301-1-ebl...@redhat.com>
> ---
> 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 <vsement...@virtuozzo.com>
--
Best regards,
Vladimir
