Re: [Qemu-devel] [PATCH 8/8] nbd: Minimal structured read for client

[Eric Blake <eblake@redhat.com>]

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

On 09/25/2017 08:58 AM, Vladimir Sementsov-Ogievskiy wrote:
> Minimal implementation: drop most of additional error information.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/nbd-client.h  |   2 +
>  include/block/nbd.h |  15 ++++-
>  block/nbd-client.c  |  97 +++++++++++++++++++++++++-----
>  nbd/client.c        | 169 +++++++++++++++++++++++++++++++++++++++++++++++-----
>  4 files changed, 249 insertions(+), 34 deletions(-)
> 

> +++ b/include/block/nbd.h
> @@ -57,11 +57,17 @@ struct NBDRequest {
>  };
>  typedef struct NBDRequest NBDRequest;
>  
> -struct NBDReply {
> +typedef struct NBDReply {
> +    bool simple;
>      uint64_t handle;
>      uint32_t error;
> -};
> -typedef struct NBDReply NBDReply;
> +
> +    uint16_t flags;
> +    uint16_t type;
> +    uint32_t tail_length;
> +    uint64_t offset;
> +    uint32_t hole_size;
> +} NBDReply;

This feels like it should be a discriminated union, rather than a struct
containing fields that are only sometimes valid...

>  
>  #define NBD_SREP_TYPE_NONE          0
>  #define NBD_SREP_TYPE_OFFSET_DATA   1
> +#define NBD_SREP_TYPE_OFFSET_HOLE   2
>  #define NBD_SREP_TYPE_ERROR         NBD_SREP_ERR(1)
> +#define NBD_SREP_TYPE_ERROR_OFFSET  NBD_SREP_ERR(2)

...especially since there is more than one type of SREP packet layout.

I also wonder why you are defining constants in a piecemeal fashion,
rather than all at once (even if your minimal server implementation
doesn't send a particular constant, there's no harm in defining them all
up front).

> +++ b/block/nbd-client.c
> @@ -179,9 +179,10 @@ err:
>      return rc;
>  }
>  
> -static int nbd_co_receive_reply(NBDClientSession *s,
> -                                uint64_t handle,
> -                                QEMUIOVector *qiov)
> +static int nbd_co_receive_1_reply_or_chunk(NBDClientSession *s,

Long name, and unusual to mix in "1" instead of "one".  Would it be
better to name it nbd_co_receive_chunk, where we declare that a simple
reply is (roughly) the same amount of effort as a chunk in a structured
reply?

> +                                           uint64_t handle,
> +                                           bool *cont,
> +                                           QEMUIOVector *qiov)
>  {

No documentation of the function?

>      int ret;
>      int i = HANDLE_TO_INDEX(s, handle);
> @@ -191,29 +192,95 @@ static int nbd_co_receive_reply(NBDClientSession *s,
>      qemu_coroutine_yield();
>      s->requests[i].receiving = false;
>      if (!s->ioc || s->quit) {
> -        ret = -EIO;
> -    } else {
> -        assert(s->reply.handle == handle);
> -        ret = -s->reply.error;
> -        if (qiov && s->reply.error == 0) {
> +        *cont = false;
> +        return -EIO;
> +    }
> +
> +    assert(s->reply.handle == handle);
> +    *cont = !(s->reply.simple || (s->reply.flags & NBD_SREP_FLAG_DONE));

We need to validate that the server is not sending us SREP chunks unless
we negotiated them.  I'm thinking it might be better to do it here
(maybe you did it somewhere else, but I haven't seen it yet; I'm
reviewing the patch in textual order rather than the order in which
things are called).

> +    ret = -s->reply.error;
> +    if (ret < 0) {
> +        goto out;
> +    }
> +
> +    if (s->reply.simple) {
> +        if (qiov) {
>              if (qio_channel_readv_all(s->ioc, qiov->iov, qiov->niov,
> -                                      NULL) < 0) {
> -                ret = -EIO;
> -                s->quit = true;
> +                                      NULL) < 0)
> +            {
> +                goto fatal;
>              }
>          }
> +        goto out;
> +    }
>  
> -        /* Tell the read handler to read another header.  */
> -        s->reply.handle = 0;
> +    /* here we deal with successful structured reply */
> +    switch (s->reply.type) {
> +        QEMUIOVector sub_qiov;
> +    case NBD_SREP_TYPE_OFFSET_DATA:

This is putting a LOT of smarts directly into the receive routine.
Here's where I was previously wondering (and I think Paolo as well)
whether it might be better to split the efforts: the generic function
reads off the chunk information and any payload, but a per-command
callback function then parses the chunks.  Especially since the
definition of the chunks differs on a per-command basis (yes, the NBD
spec will try to not reuse an SREP chunk type across multiple commands
unless the semantics are similar, but that's a bit more fragile).  This
particularly matters given my statement above that you want a
discriminated union, rather than a struct that contains unused fields,
for handling different SREP chunk types.

My review has to pause here for now...


-- 
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 --]