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
