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

[Vladimir Sementsov-Ogievskiy]

26.09.2017 01:19, Eric Blake wrote:
> 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 <address@hidden>
> > ---
> >   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...

No:

simple, handle and error are always valid
flags, type, tail_length are valid for all structured replies
offset and hole_size are valid for structured hole reply

so, we have one case when all fields are valid.. how do you see it with 
union, what is the real difference? I think it would be just a complication.

> >   
> >   #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).

hmm. just to not define unused constants. It doesn't matter, I can 
define them all if you prefer.

> > +++ 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).

No, I didn't. Will add (may be early, in reply_entry).

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

Hmm. it was my idea to move all reading into one coroutine (in my 
refactoring series, but Paolo was against).

Or you mean to read a payload as raw? It would lead to double copying it 
to destination qiov, which I dislike..

> callback function then parses the chunks.

per-command? Then callback for CMD_READ would have all of these 
"smarts", so the whole code would not be simpler.. (However it will 
simplify adding block-status later).

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


--
Best regards,
Vladimir