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

On Thu, 7 Aug 2025 14:58:34 +0200 Laurent Vivier wrote:

On 06/08/2025 04:17, David Gibson wrote:

...

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.

Stefano, why do you think of this idea?

Well, yes, it *should* be by construction, but somewhere we might eventually calculate that index (indirectly) using data we receive, and I don't think we want to ASSERT() if somebody finds a way to make us calculate a bad index. It's not a strong objection against ASSERT(), though. It makes the code marginally more terse and might help us find issues, too. I just have a slight preference for a return value in this case anyway (better to dodge a security issue and hide a functional issue than risking hitting both, I think). -- Stefano