Re: [PATCH v3 10/14] nbd/client: Initial support for extended headers

[Eric Blake <eblake@redhat.com>]

The subject lines are confusing: 9/14 enables extended headers in the
server, while this one does not yet enable the headers but is merely a
preliminary step.  I'll probably retitle one or the other in v4.

On Wed, May 31, 2023 at 06:26:17PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 15.05.23 22:53, Eric Blake wrote:
> > Update the client code to be able to send an extended request, and
> > parse an extended header from the server.  Note that since we reject
> > any structured reply with a too-large payload, we can always normalize
> > a valid header back into the compact form, so that the caller need not
> > deal with two branches of a union.  Still, until a later patch lets
> > the client negotiate extended headers, the code added here should not
> > be reached.  Note that because of the different magic numbers, it is
> > just as easy to trace and then tolerate a non-compliant server sending
> > the wrong header reply as it would be to insist that the server is
> > compliant.
> > 
> > The only caller to nbd_receive_reply() always passed NULL for errp;
> > since we are changing the signature anyways, I decided to sink the
> > decision to ignore errors one layer lower.
> 
> This way nbd_receive_simple_reply() and nbd_receive_structured_reply_chunk() are called now only with explicit NULL last argument.. And we start to drop all errors.
> 
> Also, actually, we'd better add errp parameter to the caller - nbd_receive_replies(), because its caller (nbd_co_do_receive_one_chunk()) will benefit of it, as it already has errp.

I can explore plumbing errp back through for v4.

> > @@ -1394,28 +1401,34 @@ static int nbd_receive_simple_reply(QIOChannel *ioc, NBDSimpleReply *reply,
> > 
> >   /* nbd_receive_structured_reply_chunk
> >    * Read structured reply chunk except magic field (which should be already
> > - * read).
> > + * read).  Normalize into the compact form.
> >    * Payload is not read.
> >    */
> > -static int nbd_receive_structured_reply_chunk(QIOChannel *ioc,
> > -                                              NBDStructuredReplyChunk *chunk,
> > +static int nbd_receive_structured_reply_chunk(QIOChannel *ioc, NBDReply *chunk,
> >                                                 Error **errp)
> 
> Hmm, _structured_or_extened_ ? Or at least in comment above the function we should mention this.

I'm going with 'nbd_receive_reply_chunk', since both structured and
extended modes receive chunks.

> 
> >   {
> >       int ret;
> > +    size_t len;
> > +    uint64_t payload_len;
> > 
> > -    assert(chunk->magic == NBD_STRUCTURED_REPLY_MAGIC);
> > +    if (chunk->magic == NBD_STRUCTURED_REPLY_MAGIC) {
> > +        len = sizeof(chunk->structured);
> > +    } else {
> > +        assert(chunk->magic == NBD_EXTENDED_REPLY_MAGIC);
> > +        len = sizeof(chunk->extended);
> > +    }
> > 
> >       ret = nbd_read(ioc, (uint8_t *)chunk + sizeof(chunk->magic),
> > -                   sizeof(*chunk) - sizeof(chunk->magic), "structured chunk",
> 
> Would be good to print "extended chunk" in error message for EXTENDED case.

Or even just "chunk header", which covers both modes.

> >   int coroutine_fn nbd_receive_reply(BlockDriverState *bs, QIOChannel *ioc,
> > -                                   NBDReply *reply, Error **errp)
> > +                                   NBDReply *reply, NBDHeaderStyle hdr)
> >   {
> >       int ret;
> >       const char *type;
> > 
> > -    ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), errp);
> > +    ret = nbd_read_eof(bs, ioc, &reply->magic, sizeof(reply->magic), NULL);
> >       if (ret <= 0) {
> >           return ret;
> >       }
> > 
> >       reply->magic = be32_to_cpu(reply->magic);
> > 
> > +    /* Diagnose but accept wrong-width header */
> >       switch (reply->magic) {
> >       case NBD_SIMPLE_REPLY_MAGIC:
> > -        ret = nbd_receive_simple_reply(ioc, &reply->simple, errp);
> > +        if (hdr >= NBD_HEADER_EXTENDED) {
> > +            trace_nbd_receive_wrong_header(reply->magic);
> 
> maybe, trace also expected style

Sure, I can give that a shot.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org