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

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.
> 
> [1]
https://github.com/NetworkBlockDevice/nbd/blob/extension-ext-header/doc/proto.md
> as of NBD commit e6f3b94a934
> 
> [2] Note that on the surface, this is because some future server might
> permit a 4G+ NBD_CMD_READ and need to reply with that much data in one
> transaction.  But even though the extended reply length is widened to
> 64 bits, for now the NBD spec is clear that servers will not reply
> with more than a maximum payload bounded by the 32-bit
> NBD_INFO_BLOCK_SIZE field; allowing a client and server to mutually
> agree to transactions larger than 4G would require yet another
> extension.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov at yandex-team.ru>
> ---
>   include/block/nbd.h |  8 +++---
>   nbd/server.c        | 64 ++++++++++++++++++++++++++++-----------------
>   2 files changed, 44 insertions(+), 28 deletions(-)
> 
[..]
> 
> -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.

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

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