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

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 Reviewed-by: David Gibson

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