Re: [Qemu-devel] [PATCH v2 2/5] nbd/server: fix sparse read
On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
In case of io error in nbd_co_send_sparse_read we should not
"goto reply:", as it is fatal error and common behavior is
disconnect in this case. We should not try to send client an
s/send/send the/
error reply, representing channel-io error on previous try to
send a reply.
s/representing .../since we already hit a channel-io error on our
previous attempt to send a reply/
Fix this by handle block-status error in nbd_co_send_sparse_read,
s/handle/handling/
so nbd_co_send_sparse_read fails only on io error. Then just skip
common "reply:" code path in nbd_trip.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
v2: code movement splitted to 01
remove error_report
fix indent
fix commit message subject line
add comment to nbd_co_send_sparse_read (be free to adjust it)
Not a problem, I know English is not your native tongue, so I don't mind
touching it up, and I already admire your efforts at programming in a
second language (a skill I lack).
nbd/server.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index c406b0656d..e0de431e10 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1365,6 +1365,10 @@ static int coroutine_fn
nbd_co_send_structured_error(NBDClient *client,
return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
}
+/* Do sparse read and send structured reply to the client.
s/Do/Do a/
+ * Returns -errno if sending fails. bdrv_block_status_above() fail is not
s/fail/failure/
+ * considered as an error, but reported to the client instead (as a structured
+ * error). */
The comment tail */ usually gets its own line.
Here's what I used:
/* Do a sparse read and send the structured reply to the client.
* Returns -errno if sending fails. bdrv_block_status_above() failure is
* reported to the client, at which point this function succeeds.
*/
static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
uint64_t handle,
uint64_t offset,
@@ -1385,8 +1389,13 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
bool final;
if (status < 0) {
-error_setg_errno(errp, -status, "unable to check for holes");
-return status;
+char *msg = g_strdup_printf("unable to check for holes: %s",
+strerror(-status));
I had to double-check block/io.c, but it looks like
bdrv_block-status_above() does indeed document a return of negative
errno on failure. (My suspicion is that the documentation might be
wrong, because I did not audit error returns when doing my byte-based
work; but a quick audit of a couple of random drivers shows that at
least iscsi.c and qed.c appear to comply.)
+
+ret = nbd_co_send_structured_error(client, handle, -status, msg,
+ errp);
+g_free(msg);
+return ret;
I wonder if a separate patch should clean this up to let
nbd_co_send_structured_error() do message formatting (making it varargs)
- but doesn't affect this patch.
With grammar tweaks,
Reviewed-by: Eric Blake
[Qemu-devel] [PATCH v2 2/5] nbd/server: fix sparse read
In case of io error in nbd_co_send_sparse_read we should not
"goto reply:", as it is fatal error and common behavior is
disconnect in this case. We should not try to send client an
error reply, representing channel-io error on previous try to
send a reply.
Fix this by handle block-status error in nbd_co_send_sparse_read,
so nbd_co_send_sparse_read fails only on io error. Then just skip
common "reply:" code path in nbd_trip.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
v2: code movement splitted to 01
remove error_report
fix indent
fix commit message subject line
add comment to nbd_co_send_sparse_read (be free to adjust it)
nbd/server.c | 17 ++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index c406b0656d..e0de431e10 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1365,6 +1365,10 @@ static int coroutine_fn
nbd_co_send_structured_error(NBDClient *client,
return nbd_co_send_iov(client, iov, 1 + !!iov[1].iov_len, errp);
}
+/* Do sparse read and send structured reply to the client.
+ * Returns -errno if sending fails. bdrv_block_status_above() fail is not
+ * considered as an error, but reported to the client instead (as a structured
+ * error). */
static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
uint64_t handle,
uint64_t offset,
@@ -1385,8 +1389,13 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
bool final;
if (status < 0) {
-error_setg_errno(errp, -status, "unable to check for holes");
-return status;
+char *msg = g_strdup_printf("unable to check for holes: %s",
+strerror(-status));
+
+ret = nbd_co_send_structured_error(client, handle, -status, msg,
+ errp);
+g_free(msg);
+return ret;
}
assert(pnum && pnum <= size - progress);
final = progress + pnum == size;
@@ -1567,7 +1576,7 @@ static coroutine_fn void nbd_trip(void *opaque)
request.from, req->data, request.len,
&local_err);
if (ret < 0) {
-goto reply;
+goto replied;
}
goto done;
}
@@ -1664,6 +1673,8 @@ reply:
req->data, reply_data_len, &local_err);
}
g_free(msg);
+
+replied:
if (ret < 0) {
error_prepend(&local_err, "Failed to send reply: ");
goto disconnect;
--
2.11.1
