Re: [PATCH v4 19/24] nbd/client: Initial support for extended headers

[Eric Blake]

On Tue, Jun 27, 2023 at 05:22:09PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, 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.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > @@ -1508,23 +1537,28 @@ int coroutine_fn nbd_receive_reply(BlockDriverState 
> > *bs, QIOChannel *ioc,
> >                                          reply->cookie);
        case NBD_SIMPLE_REPLY_MAGIC:
            if (mode >= NBD_MODE_EXTENDED) {
                trace_nbd_receive_wrong_header(reply->magic,
                                               nbd_mode_lookup(mode));
            }
            ret = nbd_receive_simple_reply(ioc, &reply->simple, errp);
            if (ret < 0) {
                break;
            }
            trace_nbd_receive_simple_reply(reply->simple.error,
                                           nbd_err_lookup(reply->simple.error),
                                           reply->cookie);
> >           break;
> >       case NBD_STRUCTURED_REPLY_MAGIC:
> > -        ret = nbd_receive_structured_reply_chunk(ioc, &reply->structured, 
> > errp);
> > +    case NBD_EXTENDED_REPLY_MAGIC:
> > +        expected = mode >= NBD_MODE_EXTENDED ? NBD_EXTENDED_REPLY_MAGIC
> > +            : NBD_STRUCTURED_REPLY_MAGIC;
> > +        if (reply->magic != expected) {
> > +            trace_nbd_receive_wrong_header(reply->magic,
> > +                                           nbd_mode_lookup(mode));
> > +        }
> > +        ret = nbd_receive_reply_chunk_header(ioc, reply, errp);
> >           if (ret < 0) {
> >               break;
> >           }
> 
> maybe
> 
> assert(reply->magic == NBD_STRUCTURED_REPLY_MAGIC)

No, not even as a temporary assertion.  You are correct that as of
this patch, a compliant server will only be sending structured
replies, not extended (because we haven't turned on a request for
extended headers yet); but we should never assert something that a
non-compliant server can send over the wire.  So the best we can do is
trace the server's unusual response.

> 
> >           type = nbd_reply_type_lookup(reply->structured.type);
> > -        trace_nbd_receive_structured_reply_chunk(reply->structured.flags,
> > -                                                 reply->structured.type, 
> > type,
> > -                                                 reply->structured.cookie,
> > -                                                 reply->structured.length);
> > +        trace_nbd_receive_reply_chunk_header(reply->structured.flags,
> > +                                             reply->structured.type, type,
> > +                                             reply->structured.cookie,
> > +                                             reply->structured.length);
> >           break;
> >       default:
> > +        trace_nbd_receive_wrong_header(reply->magic, 
> > nbd_mode_lookup(mode));
> >           error_setg(errp, "invalid magic (got 0x%" PRIx32 ")", 
> > reply->magic);
> >           return -EINVAL;
> >       }
> > -    if (ret < 0) {
> > -        return ret;
> > -    }
> 
> hmm, mistake? Seems, we'll return 1 on error with set errp this way
> 
> > 
> >       return 1;

Indeed, I botched that logic (either keep the 'ret < 0' filter after
the switch, or change 'break' to 'return ret' within the switch).
Will fix in v5.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org