On 3/25/19 9:44 AM, Eric Blake wrote:
>>>
>>> @@ -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.
>
> And that's okay. The whole point of this is that if the server replied
> with an error message (iter.request_ret), then it is okay that the
> server did not give us any extent->length, and the caller will turn
> iter.request_ret into the EINVAL reply (or whatever other errno value
> the server sent) to .bdrv_co_block_status() while keeping the connection
> alive. But if the server did NOT send an error reply (iter.request_ret
> is 0), but also did not send us any status, then we need to turn that
> into an error condition (because our caller expected us to make progress
> on success, even though the server did not make progress).
More to the point, the behavior of qemu for a (structured) error reply
to NBD_CMD_BLOCK_STATUS with no extent->length was to keep the
connection alive (both before and after commit 7f86068d) - the
difference in behavior for this hunk of the patch is only visible for
simple error replies. (Here's the hacks I applied to the server, to test
forced error replies to NBD_CMD_BLOCK_STATUS, whether structured or simple:
From 22b4181f887d3b782695dbb11fcc1759281fc51e Mon Sep 17 00:00:00 2001
From: Eric Blake <ebl...@redhat.com>
Date: Sat, 23 Mar 2019 07:19:33 -0500
Subject: [PATCH 1/2] nbd: Debug patch to force NBD_CMD_BLOCK_STATUS failure
Hack to test whether the client is robust to block status failure.
Signed-off-by: Eric Blake <ebl...@redhat.com>
---
nbd/server.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/nbd/server.c b/nbd/server.c
index fd013a2817a..94d0c9f4e9e 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, -EINVAL,
+ "no status for you today", errp);
if (!request->len) {
return nbd_send_generic_reply(client, request->handle, -EINVAL,
"need non-zero length", errp);
--
2.20.1
From 1f3076c31deee0f226c6af911a0b636f5cbb7faf Mon Sep 17 00:00:00 2001
From: Eric Blake <ebl...@redhat.com>
Date: Sat, 23 Mar 2019 07:19:33 -0500
Subject: [PATCH 2/2] nbd: Debug patch to force simple errors to all but
NBD_CMD_READ
When structured replies are negotiated, the NBD spec still allows
simple error replies to any command other than READ. Make it easy to
test this scenario.
Signed-off-by: Eric Blake <ebl...@redhat.com>
---
nbd/server.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 94d0c9f4e9e..35f549abbf9 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -2148,16 +2148,16 @@ static int nbd_co_receive_request(NBDRequestData
*req, NBDRequest *request,
* Returns 0 if connection is still live, -errno on failure to talk to
client
*/
static coroutine_fn int nbd_send_generic_reply(NBDClient *client,
- uint64_t handle,
+ NBDRequest *request,
int ret,
const char *error_msg,
Error **errp)
{
- if (client->structured_reply && ret < 0) {
- return nbd_co_send_structured_error(client, handle, -ret,
error_msg,
+ if (client->structured_reply && ret < 0 && request->type ==
NBD_CMD_READ) {
+ return nbd_co_send_structured_error(client, request->handle,
-ret, error_msg,
errp);
} else {
- return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0,
+ return nbd_co_send_simple_reply(client, request->handle, ret <
0 ? -ret : 0,
NULL, 0, errp);
}
}
@@ -2177,7 +2177,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient
*client, NBDRequest *request,
if (request->flags & NBD_CMD_FLAG_FUA) {
ret = blk_co_flush(exp->blk);
if (ret < 0) {
- return nbd_send_generic_reply(client, request->handle, ret,
+ return nbd_send_generic_reply(client, request, ret,
"flush failed", errp);
}
}
@@ -2192,7 +2192,7 @@ static coroutine_fn int nbd_do_cmd_read(NBDClient
*client, NBDRequest *request,
ret = blk_pread(exp->blk, request->from + exp->dev_offset, data,
request->len);
if (ret < 0 || request->type == NBD_CMD_CACHE) {
- return nbd_send_generic_reply(client, request->handle, ret,
+ return nbd_send_generic_reply(client, request, ret,
"reading from file failed", errp);
}
@@ -2234,7 +2234,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
}
ret = blk_pwrite(exp->blk, request->from + exp->dev_offset,
data, request->len, flags);
- return nbd_send_generic_reply(client, request->handle, ret,
+ return nbd_send_generic_reply(client, request, ret,
"writing to file failed", errp);
case NBD_CMD_WRITE_ZEROES:
@@ -2247,7 +2247,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
}
ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
request->len, flags);
- return nbd_send_generic_reply(client, request->handle, ret,
+ return nbd_send_generic_reply(client, request, ret,
"writing to file failed", errp);
case NBD_CMD_DISC:
@@ -2256,7 +2256,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
case NBD_CMD_FLUSH:
ret = blk_co_flush(exp->blk);
- return nbd_send_generic_reply(client, request->handle, ret,
+ return nbd_send_generic_reply(client, request, ret,
"flush failed", errp);
case NBD_CMD_TRIM:
@@ -2265,14 +2265,14 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
if (ret == 0 && request->flags & NBD_CMD_FLAG_FUA) {
ret = blk_co_flush(exp->blk);
}
- return nbd_send_generic_reply(client, request->handle, ret,
+ return nbd_send_generic_reply(client, request, ret,
"discard failed", errp);
case NBD_CMD_BLOCK_STATUS:
- return nbd_send_generic_reply(client, request->handle, -EINVAL,
+ return nbd_send_generic_reply(client, request, -EINVAL,
"no status for you today", errp);
if (!request->len) {
- return nbd_send_generic_reply(client, request->handle, -EINVAL,
+ return nbd_send_generic_reply(client, request, -EINVAL,
"need non-zero length", errp);
}
if (client->export_meta.valid &&
@@ -2306,7 +2306,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
return ret;
} else {
- return nbd_send_generic_reply(client, request->handle, -EINVAL,
+ return nbd_send_generic_reply(client, request, -EINVAL,
"CMD_BLOCK_STATUS not
negotiated",
errp);
}
@@ -2314,7 +2314,7 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
default:
msg = g_strdup_printf("invalid request type (%" PRIu32 ")
received",
request->type);
- ret = nbd_send_generic_reply(client, request->handle, -EINVAL, msg,
+ ret = nbd_send_generic_reply(client, request, -EINVAL, msg,
errp);
g_free(msg);
return ret;
@@ -2359,7 +2359,7 @@ static coroutine_fn void nbd_trip(void *opaque)
Error *export_err = local_err;
local_err = NULL;
- ret = nbd_send_generic_reply(client, request.handle, -EINVAL,
+ ret = nbd_send_generic_reply(client, &request, -EINVAL,
error_get_pretty(export_err),
&local_err);
error_free(export_err);
} else {
--
2.20.1
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
