[Libguestfs] [PATCH v3 12/14] nbd/client: Request extended headers during negotiation

[Eric Blake]

On Wed, May 31, 2023 at 08:39:53PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 15.05.23 22:53, Eric Blake wrote:
> > All the pieces are in place for a client to finally request extended
> > headers.  Note that we must not request extended headers when qemu-nbd
> 
> why must not? It should gracefully report ENOTSUP? Or not?

The kernel code does not yet know how to send extended requests; once
extended mode is negotiated, sending a simple request requires the
server to disconnect the connection because the client sent unexpected
magic of a compact request (because otherwise the server would reply
with an extended reply, with a magic number equally unexpected by the
client).  Likewise, the kernel doesn't even support structured replies
yet.

(Extending nbd.ko to support these things is something that may
eventually surface on my todo list, but it is not high priority right
now)

> 
> > is used to connect to the kernel module (as nbd.ko does not expect
> > them), but there is no harm in all other clients requesting them.
> > 
> > Extended headers are not essential to the information collected during
> > 'qemu-nbd --list', but probing for it gives us one more piece of
> > information in that output.  Update the iotests affected by the new
> > line of output.
> > 
> > Signed-off-by: Eric Blake <eblake at redhat.com>
> > ---
> >   block/nbd.c                                   |  5 +--
> >   nbd/client-connection.c                       |  2 +-
> >   nbd/client.c                                  | 38 ++++++++++++-------
> >   qemu-nbd.c                                    |  3 ++
> >   tests/qemu-iotests/223.out                    |  6 +++
> >   tests/qemu-iotests/233.out                    |  5 +++
> >   tests/qemu-iotests/241.out                    |  3 ++
> >   tests/qemu-iotests/307.out                    |  5 +++
> >   .../tests/nbd-qemu-allocation.out             |  1 +
> >   9 files changed, 51 insertions(+), 17 deletions(-)
> > 
> > diff --git a/block/nbd.c b/block/nbd.c
> > index 150dfe7170c..db107ff0806 100644
> > --- a/block/nbd.c
> > +++ b/block/nbd.c
> > @@ -1146,10 +1146,9 @@ static int coroutine_fn nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
> > 
> >           switch (chunk->type) {
> >           case NBD_REPLY_TYPE_BLOCK_STATUS_EXT:
> > -            wide = true;
> > -            /* fallthrough */
> >           case NBD_REPLY_TYPE_BLOCK_STATUS:
> > -            if (s->info.extended_headers != wide) {
> > +            wide = chunk->type == NBD_REPLY_TYPE_BLOCK_STATUS_EXT;
> > +            if ((s->info.header_style == NBD_HEADER_EXTENDED) != wide) {
> 
> O, that's a part of previous commit. Also, initialization of wide to false becomes unneeded.

Looks like rebasing got me.  I'll clean it up to minimize churn.

> > @@ -1041,8 +1049,10 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
> >       }
> > 
> >       switch (result) {
> > +    case 4: /* newstyle, with extended headers */
> >       case 3: /* newstyle, with structured replies */
> > -        info->header_style = NBD_HEADER_STRUCTURED;
> > +        /* Relies on encoding of _STRUCTURED and _EXTENDED */
> > +        info->header_style = result - 2;
> 
> I'd prefer explicit
> 
> info->header_style = (result == 4 ? NBD_HEADER_EXTENDED : NBD_HEADER_STRUCTURED);
> 
> with no comment

Can do. I also toyed with the idea of having this function return an
enum instead of an int, but then NBD_HEADER_* also needs states added
to express oldstyle.

> > @@ -1180,8 +1191,9 @@ int nbd_receive_export_list(QIOChannel *ioc, QCryptoTLSCreds *tlscreds,
> >               memset(&array[count - 1], 0, sizeof(*array));
> >               array[count - 1].name = name;
> >               array[count - 1].description = desc;
> > -            array[count - 1].header_style = result == 3 ?
> > -                NBD_HEADER_STRUCTURED : NBD_HEADER_SIMPLE;
> > +
> > +            /* Depends on values of _SIMPLE, _STRUCTURED, and _EXTENDED */
> > +            array[count - 1].header_style = result - 2;
> 
> Personally, I don't like enum-based arithmetics.. It's something to be very careful with and check enum definition every time.
> 
> Maybe, add enum NBDConnectionStyle : NBD_STYLE_OLD, NBD_STYLE_EXPORT_NAME, NBD_STYLE_SIMPLE, NBD_STYLE_STRUCTURED, NBD_STYLE_EXTENDED,
> and a simple function nbd_con_style_to_hdr_style: NBDConnectionStyle -> NBDHeaderStyle
> 
> Or, better, nbd_start_negotiate() may return only 0/1/2 as success values, and additionally set OUT argument *header_style?
> 
> And anyway, 0/1/2/3/4 - sounds like too much magic numeric logic
> 
> 
> (I feel, that's all a kind of taste and don't want to insist anyway)

Aha - you had the same idea as me - too many integer return values
without a name.  I'll figure out something that improves this area for
v4.

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