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 1F4BA5A0265 for ; Wed, 19 Oct 2022 01:54:45 +0200 (CEST) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4MsW1Y3sy4z4xGc; Wed, 19 Oct 2022 10:54:41 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1666137281; bh=y9RtOxVZxeajmcJNeu5wSCMukbOKAwcNsfMgQGRMk+8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=CIVjT94CvUIIz7gP6gVLqmQT2IEl1514jMzhiOLKE2aBEIBn2ak/cMQuTB/t4doxE AMyRzh22Bh+rcbc/8yEMbmhT0BHu8I5El4uwxHWjR+A+sxPnOvS2h7hBNjIIqpFOZo qkyBlD/spSgeNHHPHciW2Sqw2e2a61376I6swF3A= Date: Wed, 19 Oct 2022 10:54:33 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 13/14] tap: Split tap_ip4_send() into UDP and ICMP variants Message-ID: References: <20221017085807.473470-1-david@gibson.dropbear.id.au> <20221017085807.473470-14-david@gibson.dropbear.id.au> <20221018050634.428cf8d6@elisabeth> <20221018142704.11ee38ac@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="A//8iZZPABzvgS6r" Content-Disposition: inline In-Reply-To: <20221018142704.11ee38ac@elisabeth> Message-ID-Hash: DE3DPT65D5XS7BTGALWQDSXMA4KYLHFJ X-Message-ID-Hash: DE3DPT65D5XS7BTGALWQDSXMA4KYLHFJ 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.3 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: --A//8iZZPABzvgS6r Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 18, 2022 at 02:27:04PM +0200, Stefano Brivio wrote: > On Tue, 18 Oct 2022 23:07:58 +1100 > David Gibson wrote: >=20 > > On Tue, Oct 18, 2022 at 05:06:34AM +0200, Stefano Brivio wrote: > > > On Mon, 17 Oct 2022 19:58:06 +1100 > > > David Gibson wrote: > > > =20 > > > > tap_ip4_send() has special case logic to compute the checksums for = UDP > > > > and ICMP packets, which is a mild layering violation. By using a s= uitable > > > > helper we can split it into tap_udp4_send() and tap_icmp4_send() fu= nctions > > > > without greatly increasing the code size, this removing that layeri= ng > > > > violation. > > > >=20 > > > > We make some small changes to the interface while there. In both c= ases > > > > we make the destination IPv4 address a parameter, which will be use= ful > > > > later. For the UDP variant we make it take just the UDP payload, a= nd it > > > > will generate the UDP header. For the ICMP variant we pass in the = ICMP > > > > header as before. The inconsistency is because that's what seems t= o be > > > > the more natural way to invoke the function in the callers in each = case. > > > >=20 > > > > Signed-off-by: David Gibson > > > > --- > > > > icmp.c | 3 ++- > > > > tap.c | 75 +++++++++++++++++++++++++++++++++++++++++-------------= ---- > > > > tap.h | 7 ++++-- > > > > 3 files changed, 60 insertions(+), 25 deletions(-) > > > >=20 > > > > diff --git a/icmp.c b/icmp.c > > > > index 6493ea9..233acf9 100644 > > > > --- a/icmp.c > > > > +++ b/icmp.c > > > > @@ -124,7 +124,8 @@ void icmp_sock_handler(const struct ctx *c, uni= on epoll_ref ref, > > > > icmp_id_map[V4][id].seq =3D seq; > > > > } > > > > =20 > > > > - tap_ip4_send(c, sr4->sin_addr.s_addr, IPPROTO_ICMP, buf, n); > > > > + tap_icmp4_send(c, sr4->sin_addr.s_addr, tap_ip4_daddr(c), > > > > + buf, n); > > > > } > > > > } > > > > =20 > > > > diff --git a/tap.c b/tap.c > > > > index 274f4ba..5792880 100644 > > > > --- a/tap.c > > > > +++ b/tap.c > > > > @@ -127,20 +127,10 @@ static void *tap_l2_hdr(const struct ctx *c, = void *buf, uint16_t proto) > > > > return eh + 1; > > > > } > > > > =20 > > > > -/** > > > > - * tap_ip4_send() - Send IPv4 packet, with L2 headers, calculating= L3/L4 checksums > > > > - * @c: Execution context > > > > - * @src: IPv4 source address > > > > - * @proto: L4 protocol number > > > > - * @in: Payload > > > > - * @len: L4 payload length > > > > - */ > > > > -void tap_ip4_send(const struct ctx *c, in_addr_t src, uint8_t prot= o, > > > > - const char *in, size_t len) =20 > > >=20 > > > I understand why you return ip(4)h + 1 here because I've just reviewed > > > 9/14, I wouldn't know otherwise: > > >=20 > > > /** > > > * tap_ip4_hdr() - Build IPv4 header for inbound packet, with checksum > > > * @c: Execution context > > > * @src: IPv4 source address, network order > > > * @dst: IPv4 destination address, network order > > > * @len: L4 payload length > > > * @proto: L4 protocol number > > > * > > > * Return: pointer to write payload to > > > */ =20 > >=20 > > Oops, yes, forgot to add a function comment. Done. > >=20 > > > > +static void *tap_ip4_hdr(char *buf, in_addr_t src, in_addr_t dst, > > > > + size_t len, uint8_t proto) > > > > { > > > > - char buf[USHRT_MAX]; > > > > - struct iphdr *ip4h =3D (struct iphdr *)tap_l2_hdr(c, buf, ETH_P_I= P); > > > > - char *data =3D (char *)(ip4h + 1); > > > > + struct iphdr *ip4h =3D (struct iphdr *)buf; > > > > =20 > > > > ip4h->version =3D 4; > > > > ip4h->ihl =3D sizeof(struct iphdr) / 4; > > > > @@ -151,20 +141,61 @@ void tap_ip4_send(const struct ctx *c, in_add= r_t src, uint8_t proto, > > > > ip4h->ttl =3D 255; > > > > ip4h->protocol =3D proto; > > > > ip4h->saddr =3D src; > > > > - ip4h->daddr =3D tap_ip4_daddr(c); > > > > + ip4h->daddr =3D dst; > > > > csum_ip4_header(ip4h); > > > > + return ip4h + 1; > > > > +} > > > > + > > > > +/** > > > > + * tap_udp4_send() - Send UDP over IPv4 packet > > > > + * @c: Execution context > > > > + * @src: IPv4 source address > > > > + * @sport: UDP source port > > > > + * @dst: IPv4 destination address > > > > + * @dport: UDP destination port > > > > + * @in: UDP payload contents (not including UDP header) > > > > + * @len: UDP payload length (not including UDP header) > > > > + */ > > > > +/* cppcheck-suppress unusedFunction */ > > > > +void tap_udp4_send(const struct ctx *c, in_addr_t src, in_port_t s= port, > > > > + in_addr_t dst, in_port_t dport, > > > > + const void *in, size_t len) > > > > +{ > > > > + size_t udplen =3D len + sizeof(struct udphdr); > > > > + char buf[USHRT_MAX]; > > > > + void *ip4h =3D tap_l2_hdr(c, buf, ETH_P_IP); > > > > + void *uhp =3D tap_ip4_hdr(ip4h, src, dst, udplen, IPPROTO_UDP); = =20 > > >=20 > > > Two observations: > > >=20 > > > - this saves one line and one cast, but it's really a bit unnatural t= hat > > > tap_ip4_hdr() doesn't point to the header it just made, or to nothi= ng. > > >=20 > > > I would rather have to +1 the return value or the original pointer > > > instead or having this trick > > > =20 > > > > + struct udphdr *uh =3D (struct udphdr *)uhp; > > > > + char *data =3D (char *)(uh + 1); =20 > > >=20 > > > - it's longer, but in my opinion clearer, if we split a bit more clea= rly > > > the components of the packet, that is, something like (untested): = =20 > >=20 > > I don't really want to change this. Yes, it's a bit counterintuitive > > at first blush, but there's a reason for this approach. > >=20 > > This style of a function which generates a header then points *after* > > it works even if the header it generates is of variable length. > > Advancing to the payload in the caller doesn't (at least not without > > breaking the abstraction I'm trying to generate with these helpers). > >=20 > > That's not just theoretical, because at some point I'd like to extend > > the l2_hdr function to also allocate space for the qemu socket length > > header. > >=20 > > I'm certainly open to name changes to make this behaviour more > > obvious, but I think returning the payload pointer not the header > > pointer makes for a better abstraction here. >=20 > Hmm, yes, I think the variable length case is a very valid point, and > also in terms of abstraction I see the advantage. There are just two > things I can think of: >=20 > - passing the end pointer as argument (not as practical as your > solution, though) >=20 > - naming it tap_ip4_push_hdr(), tap_ip4_hdr_after(), > tap_ip4_hdr_goto_next(), tap_ip4_leave_header_behind()... I can't > think of anything better at this point. I'll keep thinking, but at the > moment I'd be fine even with the current name. I've gone with a variant of the 'push' naming, I think that makes it a bit clearer. --=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 --A//8iZZPABzvgS6r Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEoULxWu4/Ws0dB+XtgypY4gEwYSIFAmNPPLIACgkQgypY4gEw YSINPhAAibffJmR1PpHUkO5+8NRxjhHlGFEgZoRHyRUz8G2E6N3E/SvJbwUWDutV 0pTjk8cM2r6qOX8iZV2MxWs26tUrFMjBI04y/mBkZkTYovFjfFV1Ng8KvqWkgsDs wckrsvuxo7EQ5b42OArrjhst9mdHqoRcInw005XHuMfZw5OyvRnW4XDTx/Pi0Pzc dZiMP/7hFymmXZSfdWfoMg+9sVpy/newhXAEXrUGMX138r/VauAcKpV0SXFt/HXV jmc6jh5EjOIy93A6jZQ3tnTgyEkNxRapf+tyDvxkOtroJo7jCYeNy7KE2Bn8FAk1 ppPpepOIVXB2aDMHTzl0/pp+VRYh1SQxZLlqhupYA75icpeUkc0csDpTqWN7lqDf INOLeX/98lvXT4djjJ7hXpZeuMtSq3vXr7ZUhNUW8B0woq3s4846bCHJHu3V6pzG YZ9f+tp5K2+pjKxBu+YqBCwm4bBHc3bOV6JEA6aHsodeqxHX45zuth+Q3hSxXqmH Qegj8CUcegl09eH2IpvhckDDKtb5KfAJkdba09poic5B8GZOu8o8R+pd3dcA+E48 Ugwhrw+UrBagUv9qNz7VuhMWoio6+it0WrsirXNdAz+sTb2c+le1QKImW/WS6nFZ kyjWHxBR+1MdT5xk+8i3SnHawGGlUZqoh/vE6t9WGYw+g5mt9U0= =oL9Y -----END PGP SIGNATURE----- --A//8iZZPABzvgS6r--