Re: [PATCH v3 03/14] nbd/server: Prepare for alternate-size headers

[Eric Blake]

On Mon, May 29, 2023 at 05:26:50PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 15.05.23 22:53, Eric Blake wrote:
> > Upstream NBD now documents[1] an extension that supports 64-bit effect
> > lengths in requests.  As part of that extension, the size of the reply
> > headers will change in order to permit a 64-bit length in the reply
> > for symmetry[2].  Additionally, where the reply header is currently
> > 16 bytes for simple reply, and 20 bytes for structured reply; with the
> > extension enabled, there will only be one structured reply type, of 32
> > bytes.  Since we are already wired up to use iovecs, it is easiest to
> > allow for this change in header size by splitting each structured
> > reply across two iovecs, one for the header (which will become
> > variable-length in a future patch according to client negotiation),
> > and the other for the payload, and removing the header from the
> > payload struct definitions.  Interestingly, the client side code never
> > utilized the packed types, so only the server code needs to be
> > updated.
> 
> Actually, it does, since previous patch :) But, it becomes even better now? 
> Anyway some note somewhere is needed I think.

Oh, indeed - but only in a sizeof expression for an added assertion
check, and not actually for in-memory storage.

If you are envisioning a comment addition, where are you thinking it
should be placed?

> 
> > 
> > -static inline void set_be_chunk(NBDStructuredReplyChunk *chunk, uint16_t 
> > flags,
> > -                                uint16_t type, uint64_t handle, uint32_t 
> > length)
> > +static inline void set_be_chunk(NBDClient *client, struct iovec *iov,
> 
> Suggestion: pass niov here too, and caluculate "length" as a sum of iov 
> lengths starting from second extent automatically.

Makes sense.

> 
> Also, worth documenting that set_be_chunk() expects half-initialized iov, 
> with .iov_base pointing to NBDReply (IN parameter) and .iov_len unset (OUT 
> parameter), as that's not usual practice

Yeah, I'm not sure if there is a better interface, but either I come
up with one, or at least better document what I landed on.

> 
> > +                                uint16_t flags, uint16_t type,
> > +                                uint64_t handle, uint32_t length)
> >   {
> > +    NBDStructuredReplyChunk *chunk = iov->iov_base;
> > +
> > +    iov->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);
> 
> [..]
> 
> -- 
> Best regards,
> Vladimir
> 

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