[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to tra
From: |
Eric Blake |
Subject: |
Re: [Qemu-block] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen |
Date: |
Wed, 22 Nov 2017 11:08:35 -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>
The subject line says what and sort of implies why, but the commit
message body is rather sparse.
> ---
> nbd/server.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
Not a net win in lines of code, so a bit more justification may be helpful.
>
> diff --git a/nbd/server.c b/nbd/server.c
> index bccc0274e7..c9445a89e9 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -139,6 +139,19 @@ static void nbd_client_receive_next_request(NBDClient
> *client);
>
> */
>
> +static inline int nbd_opt_read(NBDClient *client, void *buffer, size_t size,
> + Error **errp)
> +{
> + client->optlen -= size;
> + return qio_channel_read_all(client->ioc, buffer, size, errp) < 0 ? -EIO
> : 0;
Should this code check that client->optlen >= size, and issue the
appropriate error message if not? That may centralize some of the error
checking repeated elsewhere.
> +}
> +
> +static inline int nbd_opt_drop(NBDClient *client, size_t size, Error **errp)
> +{
> + client->optlen -= size;
> + return nbd_drop(client->ioc, size, errp);
Same question on size validation.
> +}
> +
> /* Send a reply header, including length, but no payload.
> * Return -errno on error, 0 on success. */
> static int nbd_negotiate_send_rep_len(NBDClient *client, uint32_t type,
> @@ -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;
Here's an example caller that had a previous size validation.
> }
> @@ -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;
And again, a size validation right before the call.
> }
> be32_to_cpus(&namelen);
> - client->optlen -= sizeof(namelen);
Okay, part of the refactoring means you don't have to remember to track
remaining size separately from reading; that's a nice feature. I
suspect it is also possible to assert that client->optlen is zero before
reading the next opt (I'm reviewing the patch in order, we'll see if I
come back here). [1]
> if (namelen > client->optlen - sizeof(requests) ||
> (client->optlen - namelen) % 2)
> {
> msg = "name length is incorrect";
> goto invalid;
> }
> - if (nbd_read(client->ioc, name, namelen, errp) < 0) {
> + if (nbd_opt_read(client, name, namelen, errp) < 0) {
> return -EIO;
> }
Another size check prior to the call (actually checking multiple
conditions)...
> name[namelen] = '\0';
> - client->optlen -= namelen;
> trace_nbd_negotiate_handle_export_name_request(name);
>
> - if (nbd_read(client->ioc, &requests, sizeof(requests), errp) < 0) {
> + if (nbd_opt_read(client, &requests, sizeof(requests), errp) < 0) {
> return -EIO;
...and no direct size check before this call, because the earlier size
checks already covered it. Arguably, the in-place size check error
messages may be slightly more precise, but any message at all about
unexpected sizing is appropriate (given that only broken clients should
be sending incorrect sizes) - so I'm still leaning towards a stronger
refactoring that puts the size check in the helper function.
> @@ -521,7 +530,7 @@ static int nbd_negotiate_handle_info(NBDClient *client,
> uint16_t myflags,
> return rc;
>
> invalid:
> - if (nbd_drop(client->ioc, client->optlen, errp) < 0) {
> + if (nbd_opt_drop(client, client->optlen, errp) < 0) {
> return -EIO;
> }
> return nbd_negotiate_send_rep_err(client, NBD_REP_ERR_INVALID,
> @@ -715,7 +724,7 @@ static int nbd_negotiate_options(NBDClient *client,
> uint16_t myflags,
> return -EINVAL;
>
> default:
> - if (nbd_drop(client->ioc, length, errp) < 0) {
> + if (nbd_opt_drop(client, length, errp) < 0) {
Isn't length == client->optlen here? If so, can we omit the length
parameter to nbd_opt_drop(), and instead have it defined as dropping to
the end of the current option?
> @@ -821,6 +830,7 @@ static int nbd_negotiate_options(NBDClient *client,
> uint16_t myflags,
> if (ret < 0) {
> return ret;
> }
> + assert(client->optlen == 0);
[1] Ah, you did add the assertion.
I'm going to propose a variant on this patch, to cover the points I made
above about ease-of-use (and to hopefully show a net win in lines of code).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH 0/5] NBD server refactoring before BLOCK_STATUS, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-block] [PATCH 3/5] nbd/server: add helper nbd_opt_invalid, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-block] [PATCH 4/5] nbd: rename nbd_option and nbd_opt_reply, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-block] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-block] [PATCH 5/5] nbd/server: structurize option reply sending, Vladimir Sementsov-Ogievskiy, 2017/11/22
- [Qemu-block] [PATCH 1/5] nbd/server: refactor negotiation functions parameters, Vladimir Sementsov-Ogievskiy, 2017/11/22
- Re: [Qemu-block] [Qemu-devel] [PATCH 0/5] NBD server refactoring before BLOCK_STATUS, no-reply, 2017/11/22