Re: [PATCH v8 07/30] arp: Convert to iov_tail

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

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

On Tue, Aug 05, 2025 at 05:46:05PM +0200, Laurent Vivier wrote:
> Use packet_data() and extract headers using IOV_REMOVE_HEADER()
> rather than packet_get().
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Still R-b, but making an observation below that's perhaps more
relevant to the previous patch.

> ---
>  arp.c    | 12 +++++++++---
>  packet.c |  1 -
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index 9f1fedeafec0..b3ac42082841 100644
> --- a/arp.c
> +++ b/arp.c
> @@ -74,14 +74,20 @@ int arp(const struct ctx *c, const struct pool *p)
>  		struct arphdr ah;
>  		struct arpmsg am;
>  	} __attribute__((__packed__)) resp;
> +	struct arphdr ah_storage;
> +	struct ethhdr eh_storage;
> +	struct arpmsg am_storage;
>  	const struct ethhdr *eh;
>  	const struct arphdr *ah;
>  	const struct arpmsg *am;
> +	struct iov_tail data;
>  
> -	eh = packet_get(p, 0, 0,			 sizeof(*eh), NULL);
> -	ah = packet_get(p, 0, sizeof(*eh),		 sizeof(*ah), NULL);
> -	am = packet_get(p, 0, sizeof(*eh) + sizeof(*ah), sizeof(*am), NULL);
> +	if (!packet_data(p, 0, &data))
> +		return -1;

The only case where packet_data() will return false is if you give it
a bad packet index.  That should never happen, by construction.  So
I'm wondering if that should be an ASSSERT() in packet_data() rather
than a return value.

> +	eh = IOV_REMOVE_HEADER(&data, eh_storage);
> +	ah = IOV_REMOVE_HEADER(&data, ah_storage);
> +	am = IOV_REMOVE_HEADER(&data, am_storage);
>  	if (!eh || !ah || !am)
>  		return -1;
>  
> diff --git a/packet.c b/packet.c
> index 82adc9fd1a39..34b1722b9a03 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -201,7 +201,6 @@ void *packet_get_do(const struct pool *p, const size_t idx,
>   * Return: false if packet index is invalid, true otherwise.
>   * 	   If something wrong with @data, don't return at all (assert).
>   */
> -/* cppcheck-suppress unusedFunction */
>  bool packet_data_do(const struct pool *p, size_t idx,
>  		    struct iov_tail *data,
>  		    const char *func, int line)

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