[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] [PULL 05/17] nbd/server: refactor nbd_trip: cmd_read and ge
From: |
Eric Blake |
Subject: |
[Qemu-devel] [PULL 05/17] nbd/server: refactor nbd_trip: cmd_read and generic reply |
Date: |
Tue, 13 Mar 2018 12:13:33 -0500 |
From: Vladimir Sementsov-Ogievskiy <address@hidden>
nbd_trip has difficult logic when sending replies: 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 messages 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.
- create separate helper function nbd_send_generic_reply() and use it
both in new nbd_do_cmd_read and for other commands in nbd_trip instead
of common code-path under "reply:" label in nbd_trip. The helper
supports an error message, so logic with local_err in nbd_trip is
simplified.
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
Message-Id: <address@hidden>
Reviewed-by: Eric Blake <address@hidden>
[eblake: grammar tweaks and blank line placement]
Signed-off-by: Eric Blake <address@hidden>
---
nbd/server.c | 176 ++++++++++++++++++++++++++++++++---------------------------
1 file changed, 95 insertions(+), 81 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index b230ecb4fb8..297d314bd0b 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1521,6 +1521,72 @@ static int nbd_co_receive_request(NBDRequestData *req,
NBDRequest *request,
return 0;
}
+/* Send simple reply without a payload, or a structured error
+ * @error_msg is ignored if @ret >= 0
+ * 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,
+ 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)
+{
+ int ret;
+ NBDExport *exp = client->exp;
+
+ assert(request->type == NBD_CMD_READ);
+
+ /* XXX: NBD Protocol only documents use of FUA with WRITE */
+ 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,
+ "flush failed", errp);
+ }
+ }
+
+ if (client->structured_reply && !(request->flags & NBD_CMD_FLAG_DF) &&
+ request->len) {
+ return nbd_co_send_sparse_read(client, request->handle, request->from,
+ data, request->len, errp);
+ }
+
+ ret = blk_pread(exp->blk, request->from + exp->dev_offset, data,
+ request->len);
+ if (ret < 0) {
+ return nbd_send_generic_reply(client, request->handle, ret,
+ "reading from file failed", errp);
+ }
+
+ if (client->structured_reply) {
+ if (request->len) {
+ return nbd_co_send_structured_read(client, request->handle,
+ request->from, data,
+ request->len, true, errp);
+ } else {
+ return nbd_co_send_structured_done(client, request->handle, errp);
+ }
+ } else {
+ return nbd_co_send_simple_reply(client, request->handle, 0,
+ data, request->len, errp);
+ }
+}
+
/* Owns a reference to the NBDClient passed as opaque. */
static coroutine_fn void nbd_trip(void *opaque)
{
@@ -1530,7 +1596,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;
@@ -1558,41 +1623,23 @@ static coroutine_fn void nbd_trip(void *opaque)
}
if (ret < 0) {
- goto reply;
+ /* It wans'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:
- /* XXX: NBD Protocol only documents use of FUA with WRITE */
- if (request.flags & NBD_CMD_FLAG_FUA) {
- ret = blk_co_flush(exp->blk);
- if (ret < 0) {
- error_setg_errno(&local_err, -ret, "flush failed");
- break;
- }
- }
-
- if (client->structured_reply && !(request.flags & NBD_CMD_FLAG_DF) &&
- request.len) {
- ret = nbd_co_send_sparse_read(req->client, request.handle,
- request.from, req->data, request.len,
- &local_err);
- if (ret < 0) {
- goto replied;
- }
- goto done;
- }
-
- ret = blk_pread(exp->blk, request.from + exp->dev_offset,
- req->data, request.len);
- if (ret < 0) {
- error_setg_errno(&local_err, -ret, "reading from file failed");
- break;
- }
-
- reply_data_len = request.len;
-
+ ret = nbd_do_cmd_read(client, &request, req->data, &local_err);
break;
+
case NBD_CMD_WRITE:
flags = 0;
if (request.flags & NBD_CMD_FLAG_FUA) {
@@ -1600,11 +1647,10 @@ 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);
break;
+
case NBD_CMD_WRITE_ZEROES:
flags = 0;
if (request.flags & NBD_CMD_FLAG_FUA) {
@@ -1615,67 +1661,35 @@ static coroutine_fn void nbd_trip(void *opaque)
}
ret = blk_pwrite_zeroes(exp->blk, request.from + exp->dev_offset,
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);
break;
+
case NBD_CMD_DISC:
/* unreachable, thanks to special case in nbd_co_receive_request() */
abort();
case NBD_CMD_FLUSH:
ret = blk_co_flush(exp->blk);
- if (ret < 0) {
- error_setg_errno(&local_err, -ret, "flush failed");
- }
-
+ ret = nbd_send_generic_reply(client, request.handle, ret,
+ "flush failed", &local_err);
break;
+
case NBD_CMD_TRIM:
ret = blk_co_pdiscard(exp->blk, request.from + exp->dev_offset,
request.len);
- if (ret < 0) {
- error_setg_errno(&local_err, -ret, "discard failed");
- }
-
+ ret = nbd_send_generic_reply(client, request.handle, ret,
+ "discard failed", &local_err);
break;
+
default:
- error_setg(&local_err, "invalid request type (%" PRIu32 ") received",
- request.type);
- ret = -EINVAL;
+ 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);
}
-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;
- }
-
- if (client->structured_reply &&
- (ret < 0 || request.type == NBD_CMD_READ)) {
- if (ret < 0) {
- ret = nbd_co_send_structured_error(req->client, request.handle,
- -ret, msg, &local_err);
- } else if (reply_data_len) {
- ret = nbd_co_send_structured_read(req->client, request.handle,
- request.from, req->data,
- reply_data_len, true,
- &local_err);
- } else {
- ret = nbd_co_send_structured_done(req->client, request.handle,
- &local_err);
- }
- } else {
- ret = nbd_co_send_simple_reply(req->client, request.handle,
- ret < 0 ? -ret : 0,
- req->data, reply_data_len, &local_err);
- }
- g_free(msg);
-
replied:
if (ret < 0) {
error_prepend(&local_err, "Failed to send reply: ");
--
2.14.3
- [Qemu-devel] [PULL 00/17] NBD patches for 2018-03-13 (2.12 softfreeze), Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 04/17] nbd/server: fix: check client->closing before sending reply, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 08/17] block: let blk_add/remove_aio_context_notifier() tolerate BDS changes, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 02/17] nbd/server: move nbd_co_send_structured_error up, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 01/17] iotests: Fix stuck NBD process on 33, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 13/17] block/nbd-client: save first fatal error in nbd_iter_error, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 07/17] nbd/server: Honor FUA request on NBD_CMD_TRIM, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 14/17] nbd: BLOCK_STATUS for standard get_block_status function: client part, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 03/17] nbd/server: fix sparse read, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 06/17] nbd/server: refactor nbd_trip: split out nbd_handle_request, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 05/17] nbd/server: refactor nbd_trip: cmd_read and generic reply,
Eric Blake <=
- [Qemu-devel] [PULL 11/17] nbd/server: add nbd_read_opt_name helper, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 12/17] nbd: BLOCK_STATUS for standard get_block_status function: server part, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 10/17] nbd/server: add nbd_opt_invalid helper, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 17/17] iotests: new test 209 for NBD BLOCK_STATUS, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 09/17] iotests: add 208 nbd-server + blockdev-snapshot-sync test case, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 16/17] iotests: add file_path helper, Eric Blake, 2018/03/13
- [Qemu-devel] [PULL 15/17] iotests.py: tiny refactor: move system imports up, Eric Blake, 2018/03/13
- Re: [Qemu-devel] [PULL 00/17] NBD patches for 2018-03-13 (2.12 softfreeze), no-reply, 2018/03/13
- Re: [Qemu-devel] [PULL 00/17] NBD patches for 2018-03-13 (2.12 softfreeze), no-reply, 2018/03/13