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

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

On 31.05.23 20:54, Eric Blake wrote:
> 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
but how it could be negotiated if kernel doesn't support it?

I mean if we request extended headers during negotiation with kernel, the kernel
will just say "unsupported option", isn't it?

Or, in other words, I understand that kernel doesn't support it, I don't
understand why you note it here. Is kernel different from other NBD server
implementations which doesn't support extended requests at the moment?


-- 
Best regards,
Vladimir