Re: [PATCH v9 03/30] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6534 bytes --]

On Fri, Aug 08, 2025 at 04:01:15PM +0200, Laurent Vivier 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     | 55 +++++++++++++++++++++++++++++++++++++++++++++----------
>  iov.h     | 55 +++++++++++++++++++++++++++++++++++++++++--------------
>  tcp_buf.c |  2 +-
>  3 files changed, 87 insertions(+), 25 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index 9d282d4af461..d39bb099fa69 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -109,7 +109,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>   *
>   * Return: 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)
>  {
> @@ -127,6 +127,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) */

This suppression worries me slightly - I don't really understand why
clang would be complaining here in the first place.

[snip]
> @@ -275,7 +310,7 @@ void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align)
>   *
>   * Return: the number of elements successfully referenced from the destination
>   *	   iov array, a negative value if there is not enough room in the
> - *	   destination iov array
> + *	    destination iov array

Looks like a spurious whitespace change.

>   */
>  /* cppcheck-suppress unusedFunction */
>  ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt,
> diff --git a/iov.h b/iov.h
> index bf9820ac52ab..ccdb690ef3f1 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -70,41 +70,68 @@ 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_))

I think this belongs in the next patch instead.

>  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);
>  ssize_t iov_tail_clone(struct iovec *dst_iov, size_t dst_iov_cnt,
>  		       struct iov_tail *tail);
>  
>  /**
>   * 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.
>   *
> - * Return: pointer of type (@type_ *) located at the start of @tail_, NULL if
> - *         we can't get a contiguous and aligned pointer.
> + * Return: 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_))))
>  
>  /**
>   * 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.
>   *
> - * Return: pointer of type (@type_ *) located at the old start of @tail_, NULL
> - *         if we can't get a contiguous and aligned pointer.
> + * Return: 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
> + * @tail_:	IOV tail to remove header from (modified)
> + * @type_:	Data type of the header to remove
> + *
> + * Return: 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_))
>  
>  #endif /* IOVEC_H */
> diff --git a/tcp_buf.c b/tcp_buf.c
> index d1fca676c9a7..bc898de86919 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 th_storage, *th = IOV_REMOVE_HEADER(&tail, th_storage);
>  	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);

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]