Re: [PATCH 5/7] udp: Add udp_pktinfo() helper

[Stefano Brivio <sbrivio@redhat.com>]

I took the liberty of applying this with two minor changes:

On Tue, 15 Apr 2025 17:16:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently we open code parsing the control message for IP_PKTINFO in
> udp_peek_addr().  We have an upcoming case where we want to parse PKTINFO
> in another place, so split this out into a helper function.
> 
> While we're there, make the parsing a bit more robust: scan all cmsgs to
> look for the one we want, rather than assuming there's only one.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 52 ++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index 0bec499d..352ab83b 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -464,6 +464,41 @@ static void udp_send_tap_icmp6(const struct ctx *c,
>  	tap_icmp6_send(c, saddr, eaddr, &msg, msglen);
>  }
>  
> +/**
> + * udp_pktinfo() - Retreive packet destination address from cmsg

Nit: "Retrieve", and:

> + * @msg:	msghdr into which message has been received
> + * @dst:	(Local) destination address of message in @mh (output)
> + *
> + * Return: 0 on success, -1 if the information was missing (@dst is set to
> + *         inany_any6).
> + */
> +static int udp_pktinfo(struct msghdr *msg, union inany_addr *dst)
> +{
> +	struct cmsghdr *hdr;
> +
> +	for (hdr = CMSG_FIRSTHDR(msg); hdr; hdr = CMSG_NXTHDR(msg, hdr)) {
> +		if (hdr->cmsg_level == IPPROTO_IP &&
> +		    hdr->cmsg_type == IP_PKTINFO) {
> +			const struct in_pktinfo *i4 = (void *)CMSG_DATA(hdr);
> +
> +			*dst = inany_from_v4(i4->ipi_addr);
> +			return 0;
> +		}
> +
> +		if (hdr->cmsg_level == IPPROTO_IPV6 &&
> +			   hdr->cmsg_type == IPV6_PKTINFO) {
> +			const struct in6_pktinfo *i6 = (void *)CMSG_DATA(hdr);
> +
> +			dst->a6 = i6->ipi6_addr;
> +			return 0;
> +		}
> +	}
> +
> +	err("Missing PKTINFO cmsg on datagram");

...from a quick glance at the matching kernel path, this looks a bit
too likely for err(), so I got a bit scared, and turned it to debug().

I think warn() is actually more correct than debug() (I think it
shouldn't really be err() though because we can keep using this flow),
but I would feel a lot more confident about it once we have
rate-limited versions of those.

Of course, I'll change this back to err() or warn() if you have a
compelling reason.

Similar reasoning in 7/7 where this is called.

By the way, two messages (here and in caller) look redundant to me,
regardless of their severity, but I'm not sure if you have further
changes in mind where these separate prints would make sense.

-- 
Stefano