Re: [PATCH 3/6] ndp: Split out helpers for sending specific NDP message types

[Stefano Brivio <sbrivio@redhat.com>]

Nits only:

On Tue, 12 Nov 2024 15:06:15 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently the large ndp() function responds to all NDP messages we handle,
> both parsing the message as necessary and sending the response.  Split out
> the code to construct and send specific message types into ndp_na() (to
> send NA messages) and ndp_ra() (to send RA messages).
> 
> As well as breaking up an excessively large function, this is a first step
> to being able to send unsolicited NDP messages.
> 
> While we're there, remove a slighty ugly goto.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  ndp.c | 136 +++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 78 insertions(+), 58 deletions(-)
> 
> diff --git a/ndp.c b/ndp.c
> index fa1b67a..e876c34 100644
> --- a/ndp.c
> +++ b/ndp.c
> @@ -186,16 +186,13 @@ static void ndp_send(const struct ctx *c, const struct in6_addr *dst,
>  }
>  
>  /**
> - * ndp() - Check for NDP solicitations, reply as needed
> + * ndp_na() - Send an NDP Neighbour Advertisement (NA) message
>   * @c:		Execution context
> - * @ih:		ICMPv6 header
> - * @saddr:	Source IPv6 address
> - * @p:		Packet pool
> - *
> - * Return: 0 if not handled here, 1 if handled, -1 on failure
> + * @dst:	IPv6 address to send the NA to
> + * @addr:	IPv6 address to advertise
>   */
> -int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> -	const struct in6_addr *saddr, const struct pool *p)
> +static void ndp_na(const struct ctx *c, const struct in6_addr *dst,
> +		   const void *addr)
>  {
>  	struct ndp_na na = {
>  		.ih = {
> @@ -212,6 +209,20 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  			},
>  		}
>  	};
> +
> +	memcpy(&na.target_addr, addr, sizeof(na.target_addr));
> +	memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> +
> +	ndp_send(c, dst, &na, sizeof(na));
> +}
> +
> +/**
> + * ndp_ra() - Send an NDP Router Advertisement (RA) message
> + * @c:		Execution context
> + * @dst:	IPv6 address to send the RA to
> + */
> +static void ndp_ra(const struct ctx *c, const struct in6_addr *dst)
> +{
>  	struct ndp_ra ra = {
>  		.ih = {
>  			.icmp6_type		= RA,
> @@ -238,58 +249,28 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  			},
>  		},
>  	};
> +	unsigned char *ptr = NULL;
>  
> -	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> -		return 0;
> -
> -	if (c->no_ndp)
> -		return 1;
> -
> -	if (ih->icmp6_type == NS) {
> -		const struct ndp_ns *ns =
> -			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);
> +	memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
>  
> -		if (!ns)
> -			return -1;
> +	ptr = &ra.var[0];
>  
> -		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> -			return 1;
> -
> -		info("NDP: received NS, sending NA");
> -
> -		memcpy(&na.target_addr, &ns->target_addr,
> -		       sizeof(na.target_addr));
> -		memcpy(na.target_l2_addr.mac, c->our_tap_mac, ETH_ALEN);
> +	if (c->mtu != -1) {
> +		struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> +		*mtu = (struct opt_mtu) {
> +			.header = {
> +				.type		= OPT_MTU,
> +				.len		= 1,
> +			},
> +			.value			= htonl(c->mtu),
> +		};
> +		ptr += sizeof(struct opt_mtu);
> +	}
>  
> -		ndp_send(c, saddr, &na, sizeof(struct ndp_na));
> -	} else if (ih->icmp6_type == RS) {
> -		unsigned char *ptr = NULL;
> +	if (!c->no_dhcp_dns) {
>  		size_t dns_s_len = 0;
>  		int i, n;
>  
> -		if (c->no_ra)
> -			return 1;
> -
> -		info("NDP: received RS, sending RA");
> -		memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix));
> -
> -		ptr = &ra.var[0];
> -
> -		if (c->mtu != -1) {
> -			struct opt_mtu *mtu = (struct opt_mtu *)ptr;
> -			*mtu = (struct opt_mtu) {
> -				.header = {
> -					.type		= OPT_MTU,
> -					.len		= 1,
> -				},
> -				.value			= htonl(c->mtu),
> -			};
> -			ptr += sizeof(struct opt_mtu);
> -		}
> -
> -		if (c->no_dhcp_dns)
> -			goto dns_done;
> -
>  		for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++);
>  		if (n) {
>  			struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr;
> @@ -305,7 +286,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  				       sizeof(rdnss->dns[i]));
>  			}
>  			ptr += offsetof(struct opt_rdnss, dns) +
> -			       i * sizeof(rdnss->dns[0]);
> +				i * sizeof(rdnss->dns[0]);

Unrelated change, less readable, probably from your new editor / clangd?

>  
>  			for (n = 0; *c->dns_search[n].n; n++)
>  				dns_s_len += strlen(c->dns_search[n].n) + 2;
> @@ -329,7 +310,7 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  				*(ptr++) = '.';
>  
>  				len = sizeof(dnssl->domains) -
> -				      (ptr - dnssl->domains);
> +					(ptr - dnssl->domains);

Same here.

>  
>  				strncpy((char *)ptr, c->dns_search[i].n, len);
>  				for (dot = (char *)ptr - 1; *dot; dot++) {
> @@ -343,11 +324,50 @@ int ndp(const struct ctx *c, const struct icmp6hdr *ih,
>  			memset(ptr, 0, 8 - dns_s_len % 8);	/* padding */
>  			ptr += 8 - dns_s_len % 8;
>  		}
> +	}
>  
> -dns_done:
> -		memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
> +	memcpy(&ra.source_ll.mac, c->our_tap_mac, ETH_ALEN);
>  
> -		ndp_send(c, saddr, &ra, ptr - (unsigned char *)&ra);
> +	ndp_send(c, dst, &ra, ptr - (unsigned char *)&ra);
> +}
> +
> +/**
> + * ndp() - Check for NDP solicitations, reply as needed
> + * @c:		Execution context
> + * @ih:		ICMPv6 header
> + * @saddr:	Source IPv6 address
> + * @p:		Packet pool
> + *
> + * Return: 0 if not handled here, 1 if handled, -1 on failure
> + */
> +int ndp(const struct ctx *c, const struct icmp6hdr *ih,
> +	const struct in6_addr *saddr, const struct pool *p)
> +{
> +	if (ih->icmp6_type < RS || ih->icmp6_type > NA)
> +		return 0;
> +
> +	if (c->no_ndp)
> +		return 1;
> +
> +	if (ih->icmp6_type == NS) {
> +		const struct ndp_ns *ns =
> +			packet_get(p, 0, 0, sizeof(struct ndp_ns), NULL);

The assignment could go on its line here? We don't save any line this
way.

> +
> +		if (!ns)
> +			return -1;
> +
> +		if (IN6_IS_ADDR_UNSPECIFIED(saddr))
> +			return 1;
> +
> +		info("NDP: received NS, sending NA");
> +
> +		ndp_na(c, saddr, &ns->target_addr);
> +	} else if (ih->icmp6_type == RS) {
> +		if (c->no_ra)
> +			return 1;
> +
> +		info("NDP: received RS, sending RA");
> +		ndp_ra(c, saddr);
>  	}
>  
>  	return 1;

-- 
Stefano