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

[Vladimir Sementsov-Ogievskiy]

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>

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

> +        stq_be_p(&chunk->offset, request->from);
> +        stq_be_p(&chunk->length, length);
> +    } else {
> +        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);
> +        stq_be_p(&chunk->cookie, request->cookie);
> +        stl_be_p(&chunk->length, length);
> +    }
>   }
>
>   static int coroutine_fn nbd_co_send_chunk_done(NBDClient *client,
> @@ -2478,6 +2490,8 @@ static coroutine_fn int nbd_send_generic_reply(NBDClient 
> *client,
>   {
>       if (client->mode >= NBD_MODE_STRUCTURED && ret < 0) {
>           return nbd_co_send_chunk_error(client, request, -ret, error_msg, 
> errp);
> +    } else if (client->mode >= NBD_MODE_EXTENDED) {
> +        return nbd_co_send_chunk_done(client, request, errp);
>       } else {
>           return nbd_co_send_simple_reply(client, request, ret < 0 ? -ret : 0,
>                                           NULL, 0, errp);

--
Best regards,
Vladimir