Re: [PATCH v5 03/29] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER()

[Laurent Vivier <lvivier@redhat.com>]

On 26/05/2025 16:19, Stefano Brivio wrote:
> On Thu, 17 Apr 2025 18:51:10 +0200
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> Provide a temporary variable of the wanted type to store
>> the header if the memory in the iovec array is not contiguous.
>>
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
>> ---
>>   iov.c     | 53 ++++++++++++++++++++++++++++++++++++++++++++---------
>>   iov.h     | 52 ++++++++++++++++++++++++++++++++++++++--------------
>>   tcp_buf.c |  2 +-
>>   3 files changed, 83 insertions(+), 24 deletions(-)
>>
>> diff --git a/iov.c b/iov.c
>> index 047fcbce7fcd..907cd5339369 100644
>> --- a/iov.c
>> +++ b/iov.c
>> @@ -108,7 +108,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>>    *
>>    * Returns:    The number of bytes successfully copied.
>>    */
>> -/* cppcheck-suppress unusedFunction */
>> +/* cppcheck-suppress [staticFunction] */
>>   size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>>   		  size_t offset, void *buf, size_t bytes)
>>   {
>> @@ -126,6 +126,7 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>>   	/* copying data */
>>   	for (copied = 0; copied < bytes && i < iov_cnt; i++) {
>>   		size_t len = MIN(iov[i].iov_len - offset, bytes - copied);
>> +		/* NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker) */
>>   		memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset,
>>   		       len);
>>   		copied += len;
>> @@ -260,7 +261,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len)
>>   }
>>   
>>   /**
>> - * iov_peek_header_() - Get pointer to a header from an IOV tail
>> + * iov_check_header() - Check if a header can be accessed
>>    * @tail:	IOV tail to get header from
>>    * @len:	Length of header to get, in bytes
>>    * @align:	Required alignment of header, in bytes
>> @@ -271,8 +272,7 @@ bool iov_tail_drop(struct iov_tail *tail, size_t len)
>>    *	    overruns the IO vector, is not contiguous or doesn't have the
>>    *	    requested alignment.
>>    */
>> -/* cppcheck-suppress [staticFunction,unmatchedSuppression] */
>> -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
>> +static void *iov_check_header(struct iov_tail *tail, size_t len, size_t align)
>>   {
>>   	char *p;
>>   
>> @@ -292,27 +292,62 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
>>   	return p;
>>   }
>>   
>> +/**
>> + * iov_peek_header_() - Get pointer to a header from an IOV tail
>> + * @tail:	IOV tail to get header from
>> + * @v:		Temporary memory to use if the memory in @tail
>> + *		is discontinuous
>> + * @len:	Length of header to get, in bytes
>> + * @align:	Required alignment of header, in bytes
>> + *
>> + * @tail may be pruned, but will represent the same bytes as before.
>> + *
>> + * Returns: Pointer to the first @len logical bytes of the tail, or to
>> + *          a copy if that overruns the IO vector, is not contiguous or
>> + *          doesn't have the requested alignment. NULL if that overruns the
>> + *          IO vector.
>> + */
>> +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */
>> +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
>> +{
>> +	char *p = iov_check_header(tail, len, align);
>> +	size_t l;
>> +
>> +	if (p)
>> +		return p;
>> +
>> +	l = iov_to_buf(tail->iov, tail->cnt,  tail->off, v, len);
> 
> This effectively bypasses three checks performed by iov_check_header(),
> that is, if there's nothing left in the iov_tail, if 'len' exceeds it,
> or if it's not aligned, we'll proceed calling iov_to_buf(), whereas we
> should only call it if the buffer is not contiguous, I think.
> 
> Perhaps it would make more sense to fail on iov_check_header() failure,
> and take the contiguity check out of it, so that we preserve the early
> return on those failures.
> 
> Another alternative might be to call iov_to_buf from the old version of
> iov_peek_header_(), but I guess things would be easier to follow if
> iov_check_header() really indicates failure, instead.

I think it's correct to make a copy when it's not aligned (because it's related to the 
data position in memory, like to be not contiguous).

The only real error case we must manage is if len exceeds the remaining size in the 
iov_tail: we check the return value iov_to_buf() for that.

Thanks,
Laurent