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

[Vladimir Sementsov-Ogievskiy]

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@redhat.com>


Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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