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

[Stefano Brivio <sbrivio@redhat.com>]

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.

Nit: ... tail->cnt, tail->off ...

> +	if (l != len)
> +		return NULL;
> +
> +	return v;
> +}
> +
>  /**
>   * iov_remove_header_() - Remove a header from an IOV tail
>   * @tail:	IOV tail to remove header from (modified)
> + * @v:		Temporary memory to use if the memory in @tail
> + *		is discontinuous
>   * @len:	Length of header to remove, in bytes
>   * @align:	Required alignment of header, in bytes
>   *
>   * On success, @tail is updated so that it longer includes the bytes of the
>   * returned header.
>   *
> - * Returns: Pointer to the first @len logical bytes of the tail, NULL if that
> - *	    overruns the IO vector, is not contiguous or doesn't have the
> - *	    requested alignment.
> + * 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.
>   */
> -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align)
> +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
>  {
> -	char *p = iov_peek_header_(tail, len, align);
> +	char *p = iov_peek_header_(tail, v, len, align);
>  
>  	if (!p)
>  		return NULL;
>  
>  	tail->off = tail->off + len;
> +
>  	return p;
>  }
>  
> diff --git a/iov.h b/iov.h
> index 809f84a2c0a0..a6e0f41e8033 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -73,41 +73,65 @@ struct iov_tail {
>  #define IOV_TAIL(iov_, cnt_, off_) \
>  	(struct iov_tail){ .iov = (iov_), .cnt = (cnt_), .off = (off_) }
>  
> +/**
> + * IOV_TAIL_FROM_BUF() - Create a new IOV tail from a buffer
> + * @buf_:	Buffer address to use in the iovec
> + * @len_:	Buffer size
> + * @off_:	Byte offset in the buffer where the tail begins
> + */
> +#define IOV_TAIL_FROM_BUF(buf_, len_, off_) \
> +	IOV_TAIL((&(const struct iovec){ .iov_base = (buf_), .iov_len = (len_) }), 1, (off_))

Something like:

#define IOV_TAIL_FROM_BUF(buf_, len_, off_) \
	IOV_TAIL((&(const struct iovec){ .iov_base = (buf_),		\
					 .iov_len = (len_) }),		\
		 1,							\
		 (off_))

doesn't just have the advantage of not exceeding 80 columns, but I
think it's also more readable.

> +
>  bool iov_tail_prune(struct iov_tail *tail);
>  size_t iov_tail_size(struct iov_tail *tail);
>  bool iov_tail_drop(struct iov_tail *tail, size_t len);
> -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align);
> -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align);
> +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align);
> +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align);
>  int iov_tail_slice(struct iovec *dst_iov, size_t dst_iov_cnt,
>  		   struct iov_tail *tail, size_t *bytes);
>  
>  /**
>   * IOV_PEEK_HEADER() - Get typed pointer to a header from an IOV tail
>   * @tail_:	IOV tail to get header from
> - * @type_:	Data type of the header
> + * @var_:	Temporary buffer of the type of the header to use if
> + *		the memory in the iovec array is not contiguous.
>   *
>   * @tail_ may be pruned, but will represent the same bytes as before.
>   *
> - * Returns: Pointer of type (@type_ *) located at the start of @tail_, NULL if
> - *          we can't get a contiguous and aligned pointer.
> + * Returns: Pointer of type (@type_ *) located at the start of @tail_
> + *          or to @var_ if iovec memory is not contiguous, NULL if
> + *          that overruns the iovec.
>   */
> -#define IOV_PEEK_HEADER(tail_, type_)					\
> -	((type_ *)(iov_peek_header_((tail_),				\
> -				    sizeof(type_), __alignof__(type_))))
> +
> +#define IOV_PEEK_HEADER(tail_, var_)					\
> +	((__typeof__(var_) *)(iov_peek_header_((tail_), &(var_),	\
> +				      sizeof(var_), __alignof__(var_))))

It would be nice to preserve the usual visual alignment of arguments to
iov_peek_header_().

>  
>  /**
>   * IOV_REMOVE_HEADER() - Remove and return typed header from an IOV tail
>   * @tail_:	IOV tail to remove header from (modified)
> - * @type_:	Data type of the header to remove
> + * @var_:	Temporary buffer of the type of the header to use if
> + *		the memory in the iovec array is not contiguous.
>   *
>   * On success, @tail_ is updated so that it longer includes the bytes of the
>   * returned header.
>   *
> - * Returns: Pointer of type (@type_ *) located at the old start of @tail_, NULL
> - *          if we can't get a contiguous and aligned pointer.
> + * Returns: Pointer of type (@type_ *) located at the start of @tail_
> + *          or to @var_ if iovec memory is not contiguous, NULL if
> + *          that overruns the iovec.
> + */
> +
> +#define IOV_REMOVE_HEADER(tail_, var_)					\
> +	((__typeof__(var_) *)(iov_remove_header_((tail_), &(var_),	\
> +				    sizeof(var_), __alignof__(var_))))
> +
> +/** IOV_DROP_HEADER() -- Remove a typed header from an IOV tail

Nit: [...] - Remove ...

> + * @tail_:	IOV tail to remove header from (modified)
> + * @type_:	Data type of the header to remove
> + *
> + * Returns:	true if the tail still contains any bytes, otherwise false
>   */
> -#define IOV_REMOVE_HEADER(tail_, type_)					\
> -	((type_ *)(iov_remove_header_((tail_),				\
> -				      sizeof(type_), __alignof__(type_))))
> +#define IOV_DROP_HEADER(tail_, type_)					\
> +	iov_tail_drop((tail_), sizeof(type_))

No need for a newline now:

#define IOV_DROP_HEADER(tail_, type_)	iov_tail_drop((tail_), sizeof(type_))

>  #endif /* IOVEC_H */
> diff --git a/tcp_buf.c b/tcp_buf.c
> index 05305636b503..4bcc1acb245a 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -160,7 +160,7 @@ static void tcp_l2_buf_fill_headers(const struct tcp_tap_conn *conn,
>  				    uint32_t seq, bool no_tcp_csum)
>  {
>  	struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0);
> -	struct tcphdr *th = IOV_REMOVE_HEADER(&tail, struct tcphdr);
> +	struct tcphdr thc, *th = IOV_REMOVE_HEADER(&tail, thc);
>  	struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base;
>  	const struct flowside *tapside = TAPFLOW(conn);
>  	const struct in_addr *a4 = inany_v4(&tapside->oaddr);

-- 
Stefano