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 <ebl...@redhat.com>
> ---
>
> 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
