Re: [PATCH 09/18] udp: Convert to iov_tail

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

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

On Wed, Apr 02, 2025 at 07:23:34PM +0200, Laurent Vivier wrote:
> Use packet_base() and extract headers using IOV_REMOVE_HEADER()
> and IOV_PEEK_HEADER() rather than packet_get().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  iov.c |  1 -
>  udp.c | 37 ++++++++++++++++++++++++++-----------
>  2 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index 508fb6da91fb..0c69379316aa 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -173,7 +173,6 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt)
>   * Returns:      The number of elements successfully copied to the destination
>   *		 iov array.
>   */
> -/* cppcheck-suppress unusedFunction */
>  unsigned iov_copy(struct iovec *dst_iov, size_t dst_iov_cnt,
>  		  const struct iovec *iov, size_t iov_cnt,
>  		  size_t offset, size_t bytes)
> diff --git a/udp.c b/udp.c
> index 80520cbdf188..03906d72e75c 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -858,15 +858,20 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
>  	struct iovec m[UIO_MAXIOV];
>  	const struct udphdr *uh;
>  	struct udp_flow *uflow;
> -	int i, s, count = 0;
> +	int i, j, s, count = 0;
> +	struct iov_tail data;
>  	flow_sidx_t tosidx;
>  	in_port_t src, dst;
> +	struct udphdr uhc;
>  	uint8_t topif;
>  	socklen_t sl;
>  
>  	ASSERT(!c->no_udp);
>  
> -	uh = packet_get(p, idx, 0, sizeof(*uh), NULL);
> +	if (!packet_base(p, idx, &data))
> +		return 1;
> +
> +	uh = IOV_PEEK_HEADER(&data, uhc);
>  	if (!uh)
>  		return 1;
>  
> @@ -903,23 +908,33 @@ int udp_tap_handler(const struct ctx *c, uint8_t pif,
>  
>  	pif_sockaddr(c, &to_sa, &sl, topif, &toside->eaddr, toside->eport);
>  
> -	for (i = 0; i < (int)p->count - idx; i++) {
> -		struct udphdr *uh_send;
> -		size_t len;
> +	for (i = 0, j = 0; i < (int)p->count - idx && j < UIO_MAXIOV; i++) {
> +		const struct udphdr *uh_send;
> +
> +		if (!packet_base(p, idx + i, &data))
> +			return p->count - idx;
>  
> -		uh_send = packet_get(p, idx + i, 0, sizeof(*uh), &len);
> +		uh_send = IOV_REMOVE_HEADER(&data, uhc);
>  		if (!uh_send)
>  			return p->count - idx;
>  
> +		iov_tail_prune(&data);

Maybe we should make remove_header always prune, rather than having to
do it manually.

> +
> +		if (data.cnt + j >= UIO_MAXIOV)

Obviously it's equivalent, but this would make more intuitive sense to
me as (j + data.cnt): the amount we've already used in our array, plus
the extra we're going to need.

> +			return p->count - idx;
> +
>  		mm[i].msg_hdr.msg_name = &to_sa;
>  		mm[i].msg_hdr.msg_namelen = sl;
>  
> -		if (len) {
> -			m[i].iov_base = (char *)(uh_send + 1);
> -			m[i].iov_len = len;
> +		if (data.cnt ) {

Stray space.

> +			unsigned int len;
> +
> +			len = iov_copy(&m[j], UIO_MAXIOV - j,
> +				       &data.iov[0], data.cnt, data.off, SIZE_MAX);

So, as I mentioned on iov_copy(), it doesn't obviously report the case
where it copies less than expected not because the source is missing
data, but because the destination doesn't have enough iovecs.  You've
avoided this case with a test above, but that's a bit non-obvious, it
would be nice if we could just call this and check for an error.

> -			mm[i].msg_hdr.msg_iov = m + i;
> -			mm[i].msg_hdr.msg_iovlen = 1;
> +			mm[i].msg_hdr.msg_iov = &m[j];
> +			mm[i].msg_hdr.msg_iovlen = len;
> +			j += len;
>  		} else {
>  			mm[i].msg_hdr.msg_iov = NULL;
>  			mm[i].msg_hdr.msg_iovlen = 0;

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