Re: [PATCH v2 12/12] icmp: Dedicated functions for starting and closing ping sequences

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 21 Dec 2023 17:53:27 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> ICMP sockets are cleaned up on a timeout implemented in icmp_timer_one(),
> and the logic to do that cleanup is open coded in that function.  Similarly
> new sockets are opened when we discover we don't have an existing one in
> icmp_tap_handler(), and again the logic is open-coded.
> 
> That's not the worst thing, but it's a bit cleaner to have dedicated
> functions for the creation and destruction of ping sockets.  This will also
> make things a bit easier for future changes we have in mind.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  icmp.c | 102 +++++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 67 insertions(+), 35 deletions(-)
> 
> diff --git a/icmp.c b/icmp.c
> index 129a7f1..d669351 100644
> --- a/icmp.c
> +++ b/icmp.c
> @@ -132,6 +132,70 @@ unexpected:
>  	warn("%s: Unexpected packet on ping socket", pname);
>  }
>  
> +/**
> + * icmp_ping_close() - Close out and cleanup a ping sequence

s/cleanup/clean up/ (verb)

The commit title and this comment (referring to a sequence) are a bit
misleading... this pretty much closes the socket, and as far as I can
see setting the sequence to -1 doesn't actually have an effect (despite
the fact it's a good practice).

If you mean "sequence" not as "sequence number" but rather as a
"sequence of pings", then I think we should rename one of the two...
either 'seqnum' for the sequence number, or maybe s/sequence/socket/g
here.

> + * @c:		Execution context
> + * @id_map:	id map entry of the sequence to close
> + */
> +static void icmp_ping_close(const struct ctx *c, struct icmp_id_sock *id_map)
> +{
> +	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
> +	close(id_map->sock);
> +	id_map->sock = -1;
> +	id_map->seq = -1;
> +}
> +
> +/**
> + * icmp_ping_new() - Prepare a new ping socket for a new id
> + * @c:		Execution context
> + * @id_map:	id map entry of the sequence to open
> + * @af:		Address family, AF_INET or AF_INET6
> + * @id:		ICMP id for the new sequence
> + *
> + * Return: Newly opened ping socket fd, or -1 on failure
> + */
> +static int icmp_ping_new(const struct ctx *c, struct icmp_id_sock *id_map,
> +			 int af, uint16_t id)
> +{
> +	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
> +	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
> +	union icmp_epoll_ref iref = { .id = id };
> +	const void *bind_addr;
> +	const char *bind_if;
> +	int s;
> +
> +	if (af == AF_INET) {
> +		bind_addr = &c->ip4.addr_out;
> +		bind_if = c->ip4.ifname_out;
> +	} else {
> +		bind_addr = &c->ip6.addr_out;
> +		bind_if = c->ip6.ifname_out;
> +	}
> +
> +	s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
> +
> +	if (s < 0) {
> +		warn("Cannot open \"ping\" socket. You might need to:");
> +		warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
> +		warn("...echo requests/replies will fail.");
> +		goto cancel;
> +	}
> +
> +	if (s > FD_REF_MAX)
> +		goto cancel;
> +
> +	id_map->sock = s;
> +
> +	debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
> +
> +	return s;
> +
> +cancel:
> +	if (s >= 0)
> +		close(s);
> +	return -1;
> +}
> +
>  /**
>   * icmp_tap_handler() - Handle packets from tap
>   * @c:		Execution context
> @@ -148,7 +212,6 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
>  		     const void *saddr, const void *daddr,
>  		     const struct pool *p, const struct timespec *now)
>  {
> -	uint8_t proto = af == AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6;
>  	const char *const pname = af == AF_INET ? "ICMP" : "ICMPv6";
>  	union {
>  		struct sockaddr sa;
> @@ -200,37 +263,9 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af,
>  		ASSERT(0);
>  	}
>  
> -	if ((s = id_map->sock) < 0) {
> -		union icmp_epoll_ref iref = { .id = id };
> -		const void *bind_addr;
> -		const char *bind_if;
> -
> -		if (af == AF_INET) {
> -			bind_addr = &c->ip4.addr_out;
> -			bind_if = c->ip4.ifname_out;
> -		} else {
> -			bind_addr = &c->ip6.addr_out;
> -			bind_if = c->ip6.ifname_out;
> -		}
> -
> -		s = sock_l4(c, af, proto, bind_addr, bind_if, 0, iref.u32);
> -
> -		if (s < 0) {
> -			warn("Cannot open \"ping\" socket. You might need to:");
> -			warn("  sysctl -w net.ipv4.ping_group_range=\"0 2147483647\"");
> -			warn("...echo requests/replies will fail.");
> -			return 1;
> -		}
> -
> -		if (s > FD_REF_MAX) {
> -			close(s);
> +	if ((s = id_map->sock) < 0)
> +		if ((s = icmp_ping_new(c, id_map, af, id)) < 0)
>  			return 1;
> -		}
> -
> -		id_map->sock = s;
> -
> -		debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id);
> -	}
>  
>  	id_map->ts = now->tv_sec;
>  
> @@ -257,10 +292,7 @@ static void icmp_timer_one(const struct ctx *c, struct icmp_id_sock *id_map,
>  	if (id_map->sock < 0 || now->tv_sec - id_map->ts <= ICMP_ECHO_TIMEOUT)
>  		return;
>  
> -	epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL);
> -	close(id_map->sock);
> -	id_map->sock = -1;
> -	id_map->seq = -1;
> +	icmp_ping_close(c, id_map);
>  }
>  
>  /**

-- 
Stefano