Re: [PATCH v4 15/24] nbd/server: Prepare to send extended header replies

[Eric Blake]

On Fri, Jun 16, 2023 at 09:48:18PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 08.06.23 16:56, Eric Blake wrote:
> > Although extended mode is not yet enabled, once we do turn it on, we
> > need to reply with extended headers to all messages.  Update the low
> > level entry points necessary so that all other callers automatically
> > get the right header based on the current mode.
> > 
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > 
> > v4: new patch, split out from v3 9/14
> > ---
> >   nbd/server.c | 30 ++++++++++++++++++++++--------
> >   1 file changed, 22 insertions(+), 8 deletions(-)
> > 
> > diff --git a/nbd/server.c b/nbd/server.c
> > index 119ac765f09..84c848a31d3 100644
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1947,8 +1947,6 @@ static inline void set_be_chunk(NBDClient *client, 
> > struct iovec *iov,
> >                                   size_t niov, uint16_t flags, uint16_t 
> > type,
> >                                   NBDRequest *request)
> >   {
> > -    /* TODO - handle structured vs. extended replies */
> > -    NBDStructuredReplyChunk *chunk = iov->iov_base;
> >       size_t i, length = 0;
> > 
> >       for (i = 1; i < niov; i++) {
> > @@ -1956,12 +1954,26 @@ static inline void set_be_chunk(NBDClient *client, 
> > struct iovec *iov,
> >       }
> >       assert(length <= NBD_MAX_BUFFER_SIZE + sizeof(NBDStructuredReadData));
> > 
> > -    iov[0].iov_len = sizeof(*chunk);
> > -    stl_be_p(&chunk->magic, NBD_STRUCTURED_REPLY_MAGIC);
> > -    stw_be_p(&chunk->flags, flags);
> > -    stw_be_p(&chunk->type, type);
> > -    stq_be_p(&chunk->cookie, request->cookie);
> > -    stl_be_p(&chunk->length, length);
> > +    if (client->mode >= NBD_MODE_EXTENDED) {
> > +        NBDExtendedReplyChunk *chunk = iov->iov_base;
> > +
> > +        iov->iov_len = sizeof(*chunk);
> 
> I'd prefer to keep iov[0].iov_len notation, to stress that iov is an array
> 
> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>

I can make that change, and keep your R-b.

> 
> > +        stl_be_p(&chunk->magic, NBD_EXTENDED_REPLY_MAGIC);
> > +        stw_be_p(&chunk->flags, flags);
> > +        stw_be_p(&chunk->type, type);
> > +        stq_be_p(&chunk->cookie, request->cookie);
> 
> Hm. Not about this patch:
> 
> we now moved to simple cookies. And it seems that actually, 64bit is too much 
> for number of request.

You're right that it's more than qemu cared about.  But there may be
other implementations that really do store a 64-bit pointer as their
opaque cookie, for ease of reverse-lookup on which command the
server's reply corresponds to, so I don't see it changing any time
soon in the NBD protocol.

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