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

[Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>]

On 30.05.23 19:29, Eric Blake wrote:
> 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?

Thinking of it again, the check in 02 is incorrect originally, as it calculates NBDStructuredReplyChunk as part of payload, and with 03 it becomes correct. So, swapping 02 and 03 commits will make everything correct with no additional comments.

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

-- 
Best regards,
Vladimir