From nobody Tue Feb 10 10:04:17 2026 Delivered-To: importer@patchew.org Received-SPF: pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) client-ip=208.118.235.17; envelope-from=qemu-devel-bounces+importer=patchew.org@nongnu.org; helo=lists.gnu.org; Authentication-Results: mx.zohomail.com; spf=pass (zoho.com: domain of gnu.org designates 208.118.235.17 as permitted sender) smtp.mailfrom=qemu-devel-bounces+importer=patchew.org@nongnu.org; dmarc=fail(p=none dis=none) header.from=redhat.com Return-Path: Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) by mx.zohomail.com with SMTPS id 1520962259426178.2131039124107; Tue, 13 Mar 2018 10:30:59 -0700 (PDT) Received: from localhost ([::1]:41490 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1evnlK-0005BV-HS for importer@patchew.org; Tue, 13 Mar 2018 13:30:58 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47234) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1evnUs-0007Lz-V9 for qemu-devel@nongnu.org; Tue, 13 Mar 2018 13:14:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1evnUq-0000tE-LT for qemu-devel@nongnu.org; Tue, 13 Mar 2018 13:13:58 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:41552 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1evnUl-0000lw-62; Tue, 13 Mar 2018 13:13:51 -0400 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D3FAB818AAF8; Tue, 13 Mar 2018 17:13:50 +0000 (UTC) Received: from red.redhat.com (ovpn-121-135.rdu2.redhat.com [10.10.121.135]) by smtp.corp.redhat.com (Postfix) with ESMTP id 303F72026DFD; Tue, 13 Mar 2018 17:13:50 +0000 (UTC) From: Eric Blake To: qemu-devel@nongnu.org Date: Tue, 13 Mar 2018 12:13:33 -0500 Message-Id: <20180313171345.659672-6-eblake@redhat.com> In-Reply-To: <20180313171345.659672-1-eblake@redhat.com> References: <20180313171345.659672-1-eblake@redhat.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 13 Mar 2018 17:13:50 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 13 Mar 2018 17:13:50 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'eblake@redhat.com' RCPT:'' X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 66.187.233.73 Subject: [Qemu-devel] [PULL 05/17] nbd/server: refactor nbd_trip: cmd_read and generic reply X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Paolo Bonzini , Vladimir Sementsov-Ogievskiy , "open list:Network Block Dev..." Errors-To: qemu-devel-bounces+importer=patchew.org@nongnu.org Sender: "Qemu-devel" X-ZohoMail: RSF_0 Z_629925259 SPT_0 Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" From: Vladimir Sementsov-Ogievskiy 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 Message-Id: <20180308184636.178534-5-vsementsov@virtuozzo.com> Reviewed-by: Eric Blake [eblake: grammar tweaks and blank line placement] Signed-off-by: Eric Blake --- 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 *re= q, NBDRequest *request, return 0; } +/* Send simple reply without a payload, or a structured error + * @error_msg is ignored if @ret >=3D 0 + * Returns 0 if connection is still live, -errno on failure to talk to cli= ent + */ +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_ms= g, + 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 t= he + * client as an error reply. */ +static coroutine_fn int nbd_do_cmd_read(NBDClient *client, NBDRequest *req= uest, + uint8_t *data, Error **errp) +{ + int ret; + NBDExport *exp =3D client->exp; + + assert(request->type =3D=3D NBD_CMD_READ); + + /* XXX: NBD Protocol only documents use of FUA with WRITE */ + if (request->flags & NBD_CMD_FLAG_FUA) { + ret =3D 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->f= rom, + data, request->len, errp); + } + + ret =3D 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, er= rp); + } + } 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 =3D { 0 }; /* GCC thinks it can be used uninitia= lized */ int ret; int flags; - int reply_data_len =3D 0; Error *local_err =3D NULL; char *msg =3D 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 =3D local_err; + + local_err =3D NULL; + ret =3D 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 =3D 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 =3D 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 =3D 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 =3D request.len; - + ret =3D nbd_do_cmd_read(client, &request, req->data, &local_err); break; + case NBD_CMD_WRITE: flags =3D 0; if (request.flags & NBD_CMD_FLAG_FUA) { @@ -1600,11 +1647,10 @@ static coroutine_fn void nbd_trip(void *opaque) } ret =3D 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 =3D nbd_send_generic_reply(client, request.handle, ret, + "writing to file failed", &local_err); break; + case NBD_CMD_WRITE_ZEROES: flags =3D 0; if (request.flags & NBD_CMD_FLAG_FUA) { @@ -1615,67 +1661,35 @@ static coroutine_fn void nbd_trip(void *opaque) } ret =3D 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 =3D 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 =3D blk_co_flush(exp->blk); - if (ret < 0) { - error_setg_errno(&local_err, -ret, "flush failed"); - } - + ret =3D nbd_send_generic_reply(client, request.handle, ret, + "flush failed", &local_err); break; + case NBD_CMD_TRIM: ret =3D blk_co_pdiscard(exp->blk, request.from + exp->dev_offset, request.len); - if (ret < 0) { - error_setg_errno(&local_err, -ret, "discard failed"); - } - + ret =3D nbd_send_generic_reply(client, request.handle, ret, + "discard failed", &local_err); break; + default: - error_setg(&local_err, "invalid request type (%" PRIu32 ") receive= d", - request.type); - ret =3D -EINVAL; + msg =3D g_strdup_printf("invalid request type (%" PRIu32 ") receiv= ed", + request.type); + ret =3D nbd_send_generic_reply(client, request.handle, -EINVAL, ms= g, + &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 =3D g_strdup(error_get_pretty(local_err)); - error_report_err(local_err); - local_err =3D NULL; - } - - if (client->structured_reply && - (ret < 0 || request.type =3D=3D NBD_CMD_READ)) { - if (ret < 0) { - ret =3D nbd_co_send_structured_error(req->client, request.hand= le, - -ret, msg, &local_err); - } else if (reply_data_len) { - ret =3D nbd_co_send_structured_read(req->client, request.handl= e, - request.from, req->data, - reply_data_len, true, - &local_err); - } else { - ret =3D nbd_co_send_structured_done(req->client, request.handl= e, - &local_err); - } - } else { - ret =3D nbd_co_send_simple_reply(req->client, request.handle, - ret < 0 ? -ret : 0, - req->data, reply_data_len, &local_e= rr); - } - g_free(msg); - replied: if (ret < 0) { error_prepend(&local_err, "Failed to send reply: "); --=20 2.14.3