Re: [Libguestfs] [PATCH v3 05/14] nbd: Add types for extended headers

[Vladimir Sementsov-Ogievskiy]

On 30.05.23 21:22, Eric Blake wrote:
> On Tue, May 30, 2023 at 04:23:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > On 15.05.23 22:53, Eric Blake wrote:
> > > Add the constants and structs necessary for later patches to start
> > > implementing the NBD_OPT_EXTENDED_HEADERS extension in both the client
> > > and server, matching recent commit e6f3b94a934] in the upstream nbd
> > > project.  This patch does not change any existing behavior, but merely
> > > sets the stage.
> > >
> > > This patch does not change the status quo that neither the client nor
> > > server use a packed-struct representation for the request header.
> > >
> > > Signed-off-by: Eric Blake <eblake@redhat.com>
> >
> >
> > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> >
> > > ---
> > >    docs/interop/nbd.txt |  1 +
> > >    include/block/nbd.h  | 74 ++++++++++++++++++++++++++++++++------------
> > >    nbd/common.c         | 10 +++++-
> > >    3 files changed, 65 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
> > > index f5ca25174a6..abaf4c28a96 100644
> > > --- a/docs/interop/nbd.txt
> > > +++ b/docs/interop/nbd.txt
> > > @@ -69,3 +69,4 @@ NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE
> > >    NBD_CMD_FLAG_FAST_ZERO
> > >    * 5.2: NBD_CMD_BLOCK_STATUS for "qemu:allocation-depth"
> > >    * 7.1: NBD_FLAG_CAN_MULTI_CONN for shareable writable exports
> > > +* 8.1: NBD_OPT_EXTENDED_HEADERS
> > > diff --git a/include/block/nbd.h b/include/block/nbd.h
> > > index 50626ab2744..d753fb8006f 100644
> > > --- a/include/block/nbd.h
> > > +++ b/include/block/nbd.h
> > > @@ -87,13 +87,24 @@ typedef struct NBDStructuredReplyChunk {
> > >        uint32_t length; /* length of payload */
> > >    } QEMU_PACKED NBDStructuredReplyChunk;
> >
> > [..]
> >
> > > -/* Extent chunk for NBD_REPLY_TYPE_BLOCK_STATUS */
> > > +/* Extent array for NBD_REPLY_TYPE_BLOCK_STATUS */
> >
> > Why? NBDExtent is one extent, not extent array.
>
> It's not the entire chunk either, because that also includes the
> header and the metacontext id that are not part of the extent array.
> Maybe 'Extent array element', which matches our wire layout of:

Yes, sounds good

> <-  chunk                  ->
> <- hdr -><- payload        ->
>   ...     id  <- array      ->
>               ext[0] ext[1]...
>
> > >    typedef struct NBDExtent {
> > >        uint32_t length;
> > >        uint32_t flags; /* NBD_STATE_* */
> > >    } QEMU_PACKED NBDExtent;

--
Best regards,
Vladimir