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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 6047 bytes --]

On Sat, Jan 06, 2024 at 05:01:04PM +0100, Stefano Brivio wrote:
> 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)

Done.

> 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.

Right, I was basically meaning "flow", but avoiding it because this is
not (yet) linked to the flow table.  I've gone with
s/sequence/socket/g, more or less, and I think that works out.

> 
> > + * @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);
> >  }
> >  
> >  /**
> 

-- 
David Gibson			| 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]