[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to tra
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen |
Date: |
Wed, 22 Nov 2017 14:03:25 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 |
On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> nbd/server.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
>
Hmm, revisiting my idea about moving more of the error checking into the
helper function. As of this patch, we only have two clients of
nbd_opt_read:
> @@ -299,7 +312,7 @@ static int nbd_negotiate_handle_export_name(NBDClient
> *client,
> error_setg(errp, "Bad length received");
> return -EINVAL;
> }
> - if (nbd_read(client->ioc, name, client->optlen, errp) < 0) {
> + if (nbd_opt_read(client, name, client->optlen, errp) < 0) {
> error_prepend(errp, "read failed: ");
> return -EINVAL;
1. NBD_OPT_EXPORT_NAME, where the length check is based on the maximum
size name we're willing to accept (and NOT on comparison to the header
size, as we request the entire client->optlen). This call cannot send
an error reply (so if the length is bogus, we can just drop the
connection by returning -EINVAL). Furthermore, once we handle this
option, option processing is done; we do not reach the assert that
client->optlen == 0.
> }
> @@ -383,40 +396,36 @@ static int nbd_negotiate_handle_info(NBDClient *client,
> uint16_t myflags,
> msg = "overall request too short";
> goto invalid;
> }
> - if (nbd_read(client->ioc, &namelen, sizeof(namelen), errp) < 0) {
> + if (nbd_opt_read(client, &namelen, sizeof(namelen), errp) < 0) {
> return -EIO;
> }
2. Multiple calls within NBD_OPT_INFO/NBD_OPT_GO. Here, the length
check is based on our read being a subset of client->optlen, and our
desired response on a failed check is whatever is at the invalid: label,
namely:
invalid:
if (nbd_opt_drop(client, client->optlen, errp) < 0) {
return -EIO;
}
return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID,
errp, "%s", msg);
We want to drop all remaining data, reply to the client, and return 0 to
keep the connection alive (or -EIO if the read or write failed).
You are planning on adding more calls withing NBD_OPT_LIST_META_CONTEXT,
which will have semantics more like 2 (if we detect an inconsistent
size, we want to consume the rest of the input and return an EINVAL
reply to the client, but keep the connection alive unless there is an
I/O error in the meantime).
I think that means we need a tri-state return from nbd_opt_read(): < 0
on I/O failure, 0 if the EINVAL message has been sent, and 1 if the read
was successful; I also think it means that the handler for
NBD_OPT_EXPORT_NAME does not really fit the bill for using the same
handler. Hopefully this explanation will give you more insight into the
counter-proposal patch I'm about to post.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH 3/5] nbd/server: add helper nbd_opt_invalid, (continued)
- [Qemu-devel] [PATCH 5/5] nbd/server: structurize option reply sending, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-devel] [PATCH 4/5] nbd: rename nbd_option and nbd_opt_reply, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-devel] [PATCH 1/5] nbd/server: refactor negotiation functions parameters, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen, Vladimir Sementsov-Ogievskiy, 2017/11/22
- Re: [Qemu-devel] [PATCH 0/5] NBD server refactoring before BLOCK_STATUS, no-reply, 2017/11/22