Re: [Qemu-devel] [PATCH 2/5] nbd/server: add nbd_opt_{read, drop} to track client->optlen

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5322 bytes --]

On 11/22/2017 04:19 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]