Re: [PATCH v2 06/13] iov: Add IOV_PUT_HEADER() to write header data back to iov_tail

[Laurent Vivier <lvivier@redhat.com>]

On 3/12/26 05:12, David Gibson wrote:
> On Mon, Mar 09, 2026 at 10:47:37AM +0100, Laurent Vivier wrote:
>> Add a counterpart to IOV_PEEK_HEADER() that writes header data back
>> to an iov_tail after modification. If the header pointer matches the
>> original iov buffer location, it only advances the offset. Otherwise,
>> it copies the data using iov_from_buf().
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Although a couple of thoughts on possible improvements below.
> 
>> ---
>>   iov.c | 24 ++++++++++++++++++++++++
>>   iov.h | 14 +++++++++++++-
>>   2 files changed, 37 insertions(+), 1 deletion(-)
>>
>> diff --git a/iov.c b/iov.c
>> index cd48667226f3..296f24b61067 100644
>> --- a/iov.c
>> +++ b/iov.c
>> @@ -305,6 +305,30 @@ void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
>>   	return v;
>>   }
>>   
>> +/**
>> + * iov_put_header_() - Write header back to an IOV tail
>> + * @tail:	IOV tail to write header to
>> + * @v:		Pointer to header data to write
>> + * @len:	Length of header to write, in bytes
>> + *
>> + * Return: number of bytes written
>> + */
>> +/* cppcheck-suppress unusedFunction */
>> +size_t iov_put_header_(struct iov_tail *tail, const void *v, size_t len)
>> +{
>> +	size_t l = len;
>> +
>> +	/* iov_peek_header_() already called iov_check_header() */
>> +	if ((char *)tail->iov[0].iov_base + tail->off != v)
>> +		l = iov_from_buf(tail->iov, tail->cnt, tail->off, v, len);
> 
> This assumes the tail is already pruned.  That will be the case
> assuming this has a matching iov_peek_header_(), but it might be
> better to explicitly document that assumption.
> 
> Better yet, is there a way we could enforce these come in pairs.
> Something like:
> 
> 	struct some_header *hdr;
> 
> 	with_header(hdr, tail) {
> 		hdr->a = 123;
> 		hdr->b = hdr->c - 7;
> 	}
> 
> Where,
>      #define with_header(hdr, tail)
> 	for ((typeof *(hdr)) store, hdr = IOV_PEEK_HEADER(tail,	store);
> 	     hdr;
> 	     IOV_PUT_HEADER(tail, hdr), hdr = NULL)
> 

I really like the idea.

Thanks,
Laurent