From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 5A5CF5A026D for ; Sun, 7 Jan 2024 05:30:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1704601841; bh=xltefmr8Kqpaxl2s5wwPf9BSXIfzcz4ZmfZi4yAbTN0=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ckvQy87MrPHAQNKNdV93KlQqbhZIM6FzToHzeiAIEpDT3MvDJGH7CugTjo/1A1je+ YuHe2ezl/AvBWhZHpdvG6iDfS1wAFqj752ppKGfNJ9CfQu8uk/ARnl8eQ84RCzeamc JcrCWK9g6eUgELR90D5m+urpekJKJxcq4bLy0VivSO8tFJJPuw0a1PbOftTqHacKxw iCGnj5/P8n1sqtl1kFkqFvlkQAWgyZWa8IO9aiPG5CcBNNsFMAz/zd++BanOspvWif f0bOk2I1OdNNmuROwz9uyP1EsGF7mDOj7qr7v2/esusuovZ2ePfXRQfSQuwAENotNe caq9BonrJnsLw== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4T744d0r41z4wch; Sun, 7 Jan 2024 15:30:41 +1100 (AEDT) Date: Sun, 7 Jan 2024 15:30:32 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH v2 12/12] icmp: Dedicated functions for starting and closing ping sequences Message-ID: References: <20231221065327.1307827-1-david@gibson.dropbear.id.au> <20231221065327.1307827-13-david@gibson.dropbear.id.au> <20240106170104.48d4518e@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="oNB6/jNPQL3uSPha" Content-Disposition: inline In-Reply-To: <20240106170104.48d4518e@elisabeth> Message-ID-Hash: ILJGHRY6QX5BSZTXWWHQPVPT2OVD5FBX X-Message-ID-Hash: ILJGHRY6QX5BSZTXWWHQPVPT2OVD5FBX X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --oNB6/jNPQL3uSPha Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jan 06, 2024 at 05:01:04PM +0100, Stefano Brivio wrote: > On Thu, 21 Dec 2023 17:53:27 +1100 > David Gibson wrote: >=20 > > 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. Simil= arly > > 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. > >=20 > > 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. > >=20 > > Signed-off-by: David Gibson > > --- > > icmp.c | 102 +++++++++++++++++++++++++++++++++++++-------------------- > > 1 file changed, 67 insertions(+), 35 deletions(-) > >=20 > > 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); > > } > > =20 > > +/** > > + * icmp_ping_close() - Close out and cleanup a ping sequence >=20 > 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). >=20 > 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. >=20 > > + * @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 =3D -1; > > + id_map->seq =3D -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 =3D af =3D=3D AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6; > > + const char *const pname =3D af =3D=3D AF_INET ? "ICMP" : "ICMPv6"; > > + union icmp_epoll_ref iref =3D { .id =3D id }; > > + const void *bind_addr; > > + const char *bind_if; > > + int s; > > + > > + if (af =3D=3D AF_INET) { > > + bind_addr =3D &c->ip4.addr_out; > > + bind_if =3D c->ip4.ifname_out; > > + } else { > > + bind_addr =3D &c->ip6.addr_out; > > + bind_if =3D c->ip6.ifname_out; > > + } > > + > > + s =3D 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=3D\"0 2147483647\""); > > + warn("...echo requests/replies will fail."); > > + goto cancel; > > + } > > + > > + if (s > FD_REF_MAX) > > + goto cancel; > > + > > + id_map->sock =3D s; > > + > > + debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id); > > + > > + return s; > > + > > +cancel: > > + if (s >=3D 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 p= if, int af, > > const void *saddr, const void *daddr, > > const struct pool *p, const struct timespec *now) > > { > > - uint8_t proto =3D af =3D=3D AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6; > > const char *const pname =3D af =3D=3D 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); > > } > > =20 > > - if ((s =3D id_map->sock) < 0) { > > - union icmp_epoll_ref iref =3D { .id =3D id }; > > - const void *bind_addr; > > - const char *bind_if; > > - > > - if (af =3D=3D AF_INET) { > > - bind_addr =3D &c->ip4.addr_out; > > - bind_if =3D c->ip4.ifname_out; > > - } else { > > - bind_addr =3D &c->ip6.addr_out; > > - bind_if =3D c->ip6.ifname_out; > > - } > > - > > - s =3D 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=3D\"0 2147483647\""); > > - warn("...echo requests/replies will fail."); > > - return 1; > > - } > > - > > - if (s > FD_REF_MAX) { > > - close(s); > > + if ((s =3D id_map->sock) < 0) > > + if ((s =3D icmp_ping_new(c, id_map, af, id)) < 0) > > return 1; > > - } > > - > > - id_map->sock =3D s; > > - > > - debug("%s: new socket %i for echo ID %"PRIu16, pname, s, id); > > - } > > =20 > > id_map->ts =3D now->tv_sec; > > =20 > > @@ -257,10 +292,7 @@ static void icmp_timer_one(const struct ctx *c, st= ruct icmp_id_sock *id_map, > > if (id_map->sock < 0 || now->tv_sec - id_map->ts <=3D ICMP_ECHO_TIMEO= UT) > > return; > > =20 > > - epoll_ctl(c->epollfd, EPOLL_CTL_DEL, id_map->sock, NULL); > > - close(id_map->sock); > > - id_map->sock =3D -1; > > - id_map->seq =3D -1; > > + icmp_ping_close(c, id_map); > > } > > =20 > > /** >=20 --=20 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 --oNB6/jNPQL3uSPha Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmWaKNcACgkQzQJF27ox 2GeAog/9HkStFmK6kIvU4pUiBManUwlRX4w03gFa29ERyTXfTw6H8zmVR8sUp9v3 RHjqiNkX2/DMrS4f5MgevZaC5ctr5oCQ+EECo8qa28VaDVnbu7CCOG2crayoPvze ULTrBPzddP2uuRCJez9W5QtEn0RUFqcHkoodmdxgOPq2cHl7A7aXXrhqLjWIbr7E 0ct3XdTDF/w3PD6srw/c8Or9TJ4GvtVQrA5A3/naCW1xaN9qmHV/fHDLd+bpnSw/ sPDk2cJTCcUkKu6N8ov6yTu2KbFjyYcz7V5BBtT0IkFpjyZ/MAHMW5FS1oaZq8Ji x/2DvUBPrfs5CfZzpjqKgzYJp8hZTfhTNB6vzrPSIY55Uj4loz3c0fBSFsjOBdoA MFxxEql/maCpfX/3YWAb7sVBLbgp0Xukn8gsOQZOXaOOUBUzrvrPvDt3Z4+HtaYJ 1qrWV/JbTtmg49RovkJN3exz+2kkdUfDU80CN9q8qdTbY5q8ZyneWasUWn59xZl4 pzuwCtZOwIRNYNaq1FZZYlS31al1uoIZvqu+5A9DtF0wU4mqYBF1gM/aKb2EBKY5 UZuiNwsD49asuYrnWhNsqu1iK+kXoxPtFuB1yD+lbHV0gqKOrTiDusyGcjCETyAb kGsNnCVbGfnWl+K/ypC6mDWClWSgppfogrvvW0Uy5FPSmRZKtpg= =Ktl8 -----END PGP SIGNATURE----- --oNB6/jNPQL3uSPha--