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

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?

> 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(a)redhat.com&gt; > --- > 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.

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

> @@ -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)