[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 4/5] nbd/server: refactor nbd_trip: cmd_read and
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH 4/5] nbd/server: refactor nbd_trip: cmd_read and generic reply |
Date: |
Fri, 9 Mar 2018 16:21:32 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
On 03/08/2018 12:46 PM, Vladimir Sementsov-Ogievskiy wrote:
nbd_trip has difficult logic of sending reply: it tries to use one
code path for all replies. It is ok for simple replies, but is not
comfortable for structured replies. Also, two types of error (and
corresponding message in local_err) - fatal (leading to disconnect)
and not-fatal (just to be sent to the client) are difficult to follow.
To make things a bit clearer, the following is done:
- split CMD_READ logic to separate function. It is the most difficult
command for now, and it is definitely cramped inside nbd_trip. Also,
it is difficult to follow CMD_READ logic, shared between
"case NBD_CMD_READ" and "if"s under "reply:" label.
Yay - I already admitted when adding sparse read replies that splitting
the logic was weird.
- create separate helper function nbd_send_generic_reply() and use it
both in new nbd_do_cmd_read and for other command in nbd_trip instead
s/command/commands/
of common code-path under "reply:" label in nbd_trip. The helper
supports error message, so logic with local_err in nbd_trip is
s/supports/supports an/
simplified.
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
nbd/server.c | 164 ++++++++++++++++++++++++++++++++---------------------------
1 file changed, 88 insertions(+), 76 deletions(-)
Gains a few lines, but I think the end result is more legible.
diff --git a/nbd/server.c b/nbd/server.c
index 97b45a21fa..a2f5f73d52 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1520,6 +1520,70 @@ static int nbd_co_receive_request(NBDRequestData *req,
NBDRequest *request,
return 0;
}
+/* Send simple reply without a payload or structured error
s/payload or/payload, or a /
+ * @error_msg is ignored if @ret >= 0 */
Maybe mention the return value (0 if connection is still live, -errno on
failure to communicate to client)
+static coroutine_fn int nbd_send_generic_reply(NBDClient *client,
+ uint64_t handle,
+ 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,
+ errp);
+ } else {
+ return nbd_co_send_simple_reply(client, handle, ret < 0 ? -ret : 0,
+ NULL, 0, errp);
+ }
+}
+
+/* Handle NBD_CMD_READ request.
+ * Return -errno if sending fails. Other errors are reported directly to the
+ * client as an error reply. */
+static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *request,
+ uint8_t *data, Error **errp)
+{
+
/* Owns a reference to the NBDClient passed as opaque. */
static coroutine_fn void nbd_trip(void *opaque)
{
@@ -1529,7 +1593,6 @@ static coroutine_fn void nbd_trip(void *opaque)
NBDRequest request = { 0 }; /* GCC thinks it can be used uninitialized
*/
int ret;
int flags;
- int reply_data_len = 0;
Error *local_err = NULL;
char *msg = NULL;
@@ -1556,39 +1619,21 @@ static coroutine_fn void nbd_trip(void *opaque)
}
if (ret < 0) {
- goto reply;
+ /* It's not a -EIO, so, accordingly to nbd_co_receive_request()
It wasn't -EIO, so according to nbd_co_receive_request()
+ * semantics, we should return the error to the client. */
+ Error *export_err = local_err;
+
+ local_err = NULL;
+ ret = nbd_send_generic_reply(client, request.handle, -EINVAL,
+ error_get_pretty(export_err), &local_err);
+ error_free(export_err);
+
+ goto replied;
}
switch (request.type) {
case NBD_CMD_READ:
- reply_data_len = request.len;
+ ret = nbd_do_cmd_read(client, &request, req->data, &local_err);
break;
We could also collapse the empty line before break.
case NBD_CMD_WRITE:
@@ -1598,9 +1643,8 @@ static coroutine_fn void nbd_trip(void *opaque)
}
ret = blk_pwrite(exp->blk, request.from + exp->dev_offset,
req->data, request.len, flags);
- if (ret < 0) {
- error_setg_errno(&local_err, -ret, "writing to file failed");
- }
+ ret = nbd_send_generic_reply(client, request.handle, ret,
+ "writing to file failed", &local_err);
I like how this works out.
break;
default:
- error_setg(&local_err, "invalid request type (%" PRIu32 ") received",
- request.type);
- ret = -EINVAL;
- }
-
-reply:
- if (local_err) {
- /* If we get here, local_err was not a fatal error, and should be sent
- * to the client. */
- assert(ret < 0);
- msg = g_strdup(error_get_pretty(local_err));
- error_report_err(local_err);
- local_err = NULL;
+ msg = g_strdup_printf("invalid request type (%" PRIu32 ") received",
+ request.type);
+ ret = nbd_send_generic_reply(client, request.handle, -EINVAL, msg,
+ &local_err);
+ g_free(msg);
I wonder if nbd_send_generic_reply() would be any easier to use if it
took varargs and formatted the argument. But for now, this is okay.
Only comment and grammar tweaks, so
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
- [Qemu-block] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS, Vladimir Sementsov-Ogievskiy, 2018/03/08
- [Qemu-block] [PATCH 4/5] nbd/server: refactor nbd_trip: cmd_read and generic reply, Vladimir Sementsov-Ogievskiy, 2018/03/08
- Re: [Qemu-block] [PATCH 4/5] nbd/server: refactor nbd_trip: cmd_read and generic reply,
Eric Blake <=
- [Qemu-block] [PATCH 3/5] nbd/server: fix: check client->closing before reply sending, Vladimir Sementsov-Ogievskiy, 2018/03/08
- [Qemu-block] [PATCH 1/5] nbd/server: move nbd_co_send_structured_error up, Vladimir Sementsov-Ogievskiy, 2018/03/08
- [Qemu-block] [PATCH v2 2/5] nbd/server: fix sparse read, Vladimir Sementsov-Ogievskiy, 2018/03/08
- [Qemu-block] [PATCH 5/5] nbd/server: refactor nbd_trip: split out nbd_handle_request, Vladimir Sementsov-Ogievskiy, 2018/03/08
- Re: [Qemu-block] [PATCH 0/5] nbd server fixing and refactoring before BLOCK_STATUS, Eric Blake, 2018/03/09