public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Laurent Vivier <lvivier@redhat.com>
Cc: passt-dev@passt.top
Subject: Re: [PATCH v8 03/30] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER()
Date: Wed, 6 Aug 2025 11:45:23 +1000	[thread overview]
Message-ID: <aJKzs4NYpw5bFC1z@zatzit> (raw)
In-Reply-To: <20250805154628.301343-4-lvivier@redhat.com>

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

On Tue, Aug 05, 2025 at 05:46:01PM +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) */

Which parameter was cppcheck complaining about?

>  		memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset,
>  		       len);
>  		copied += len;
> @@ -208,7 +209,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
> @@ -219,8 +220,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;
>  
> @@ -240,27 +240,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.
> + *
> + * Return: 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);
> +	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.
>   *
> - * Return: 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.
> + * Return: 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;

Do you want to use your new iov_tail_drop() here?

> +
>  	return p;
>  }
>  
> @@ -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
>   */
>  /* 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_))
> +

This seems unrelated to the rest of the patch, does it belong in a
different one?

Also, given that you're constructing the iovec entries yourself, why
not set tail->off to zero and update iov_base and len accordingly.

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

The NULL case can't happen if given a variable of the right type.

>   */
> -#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.

Again, NULL case can't happen.

> + */
> +
> +#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 --]

  reply	other threads:[~2025-08-06  1:58 UTC|newest]

Thread overview: 66+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-08-05 15:45 [PATCH v8 00/30] Introduce discontiguous frames management Laurent Vivier
2025-08-05 15:45 ` [PATCH v8 01/30] arp: Don't mix incoming and outgoing buffers Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 02/30] iov: Introduce iov_tail_clone() and iov_tail_drop() Laurent Vivier
2025-08-06  1:32   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 03/30] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER() Laurent Vivier
2025-08-06  1:45   ` David Gibson [this message]
2025-08-05 15:46 ` [PATCH v8 04/30] tap: Use iov_tail with tap_add_packet() Laurent Vivier
2025-08-06  1:56   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 05/30] packet: Use iov_tail with packet_add() Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 06/30] packet: Add packet_data() Laurent Vivier
2025-08-06  2:14   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 07/30] arp: Convert to iov_tail Laurent Vivier
2025-08-06  2:17   ` David Gibson
2025-08-07 12:58     ` Laurent Vivier
2025-08-07 13:11       ` Stefano Brivio
2025-08-13  2:21         ` David Gibson
2025-08-05 15:46 ` [PATCH v8 08/30] ndp: " Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 09/30] icmp: " Laurent Vivier
2025-08-06  2:20   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 10/30] udp: " Laurent Vivier
2025-08-06  2:23   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 11/30] tcp: Convert tcp_tap_handler() to use iov_tail Laurent Vivier
2025-08-06  2:35   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 12/30] tcp: Convert tcp_data_from_tap() " Laurent Vivier
2025-08-06  2:37   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 13/30] dhcpv6: move offset initialization out of dhcpv6_opt() Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 14/30] dhcpv6: Extract sending of NotOnLink status Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 15/30] dhcpv6: Convert to iov_tail Laurent Vivier
2025-08-05 15:46 ` [PATCH v8 16/30] dhcpv6: Use iov_tail in dhcpv6_opt() Laurent Vivier
2025-08-06  4:14   ` David Gibson
2025-08-08 13:59     ` Laurent Vivier
2025-08-13  2:29       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 17/30] dhcp: Convert to iov_tail Laurent Vivier
2025-08-06  4:38   ` David Gibson
2025-08-08  9:33     ` Laurent Vivier
2025-08-13  2:27       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 18/30] ip: Use iov_tail in ipv6_l4hdr() Laurent Vivier
2025-08-06  5:12   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 19/30] tap: Convert tap4_handler() to iov_tail Laurent Vivier
2025-08-06  5:17   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 20/30] tap: Convert tap6_handler() " Laurent Vivier
2025-08-06  6:21   ` David Gibson
2025-08-08 13:57     ` Laurent Vivier
2025-08-13  3:22       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 21/30] packet: rename packet_data() to packet_get() Laurent Vivier
2025-08-06  6:22   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 22/30] arp: use iov_tail rather than pool Laurent Vivier
2025-08-06  6:24   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 23/30] dhcp: " Laurent Vivier
2025-08-06  6:26   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 24/30] dhcpv6: " Laurent Vivier
2025-08-06  6:27   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 25/30] icmp: " Laurent Vivier
2025-08-06  6:29   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 26/30] ndp: " Laurent Vivier
2025-08-06  6:31   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 27/30] packet: remove PACKET_POOL() and PACKET_POOL_P() Laurent Vivier
2025-08-06  6:32   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 28/30] packet: remove unused parameter from PACKET_POOL_DECL() Laurent Vivier
2025-08-06  6:33   ` David Gibson
2025-08-05 15:46 ` [PATCH v8 29/30] packet: Refactor vhost-user memory region handling Laurent Vivier
2025-08-07  6:10   ` David Gibson
2025-08-07  9:05     ` Laurent Vivier
2025-08-07 11:44       ` David Gibson
2025-08-05 15:46 ` [PATCH v8 30/30] packet: Add support for multi-vector packets Laurent Vivier
2025-08-07  6:17   ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aJKzs4NYpw5bFC1z@zatzit \
    --to=david@gibson.dropbear.id.au \
    --cc=lvivier@redhat.com \
    --cc=passt-dev@passt.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).