From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 70F8E5A02E8 for ; Wed, 1 May 2024 02:10:35 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1714522228; bh=EJcfXYnlDBGvxLd73GslGPy9Zf9dl0G4bWqwlhRkKnE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=B0xVb2Gc6SeStSBrvfBRqNvREvbj9oAgrDmdm+y+mlVuWZPBTeA2d3sCR2+jvh4pA kkeJ1qO8ZMM3mMicLze8YP58NDIlcodSVg6J0O3NNL31jI0OhCeJ+RR1McCDX6fLPj +MkJmJFLZXciWIKWJFj1qnf+psRSU5dy4suB41JvDAjli5Rr3rCpYHOL9StvVhmPWr neD3UW0Prq5kUj93D1VC0MdXc6GIOoM6bBITzhGLe4F6rc/V5jCDJW8LvxNmt0DM+A eFduqVLhz45rAbstTLP4KdRSbaZNVSBPhsmOa/S7fnc5fU4+9h36ToTEyMoF3BpMOu V8CZLU4gVgX3g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VTcsJ2tv2z4wxs; Wed, 1 May 2024 10:10:28 +1000 (AEST) Date: Wed, 1 May 2024 10:05:21 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 3/7] treewide: Standardise variable names for various packet lengths Message-ID: References: <20240429070933.1366881-1-david@gibson.dropbear.id.au> <20240429070933.1366881-4-david@gibson.dropbear.id.au> <20240430204654.41bdb93e@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="q7FfBkf6UVS1CBat" Content-Disposition: inline In-Reply-To: <20240430204654.41bdb93e@elisabeth> Message-ID-Hash: X5LK44S5T6E3SKEUZDMQRWFANEIHRBFG X-Message-ID-Hash: X5LK44S5T6E3SKEUZDMQRWFANEIHRBFG 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, Laurent Vivier 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: --q7FfBkf6UVS1CBat Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Apr 30, 2024 at 08:46:54PM +0200, Stefano Brivio wrote: > On Mon, 29 Apr 2024 17:09:29 +1000 > David Gibson wrote: >=20 > > At various points we need to track the lengths of a packet including or > > excluding various different sets of headers. We don't always use the s= ame > > variable names for doing so. Worse in some places we use the same name > > for different things: e.g. tcp_fill_headers[46]() use ip_len for the > > length including the IP headers, but then tcp_send_flag() which calls it > > uses it to mean the IP payload length only. > >=20 > > To improve clarity, standardise on these names: > > plen: L4 protocol payload length > > l4len: plen + length of L4 protocol header > > l3len: l4len + length of IPv4/IPv6 header >=20 > If we're dealing with IPv4, I guess tot_len is still a reasonable > synonym for this, as you left for csum_ip4_header(). I'd prefer to stick with l3len, so we're consistent across both IPv4 and IPv6. csum_ip4_header() is a special case, becase.. >=20 > > l2len: l3len + length of L2 (ethernet) header > >=20 > > Signed-off-by: David Gibson > > --- > > checksum.c | 42 +++++++++++++++++----------------- > > checksum.h | 10 ++++----- > > icmp.c | 8 +++---- > > passt.h | 4 ++-- > > tap.c | 48 +++++++++++++++++++-------------------- > > tcp.c | 66 +++++++++++++++++++++++++++--------------------------- > > udp.c | 24 ++++++++++---------- > > 7 files changed, 101 insertions(+), 101 deletions(-) > >=20 > > diff --git a/checksum.c b/checksum.c > > index 9cbe0b29..3602215a 100644 > > --- a/checksum.c > > +++ b/checksum.c > > @@ -116,7 +116,7 @@ uint16_t csum_fold(uint32_t sum) > > =20 > > /** > > * csum_ip4_header() - Calculate IPv4 header checksum > > - * @tot_len: IPv4 payload length (data + IP header, network order) > > + * @tot_len: IPv4 packet length (data + IP header, network order) =2E.the value here is network order. I'm sticking with "tot_len" to emphasise that this is exactly the same bytes as the header field, rather than a logical integer value. I don't love that - I never like passing integers in non-native endianness - but I prefer it to using the standard name for a value that doesn't have standard encoding. I'm considering a separate cleanup for the endianness here, but that's out of scope for this patch. > > * @protocol: Protocol number (network order) > > * @saddr: IPv4 source address (network order) > > * @daddr: IPv4 destination address (network order) > > @@ -140,13 +140,13 @@ uint16_t csum_ip4_header(uint16_t tot_len, uint8_= t protocol, > > /** > > * proto_ipv4_header_psum() - Calculates the partial checksum of an > > * IPv4 header for UDP or TCP > > - * @tot_len: IPv4 Payload length (host order) > > + * @l4len: IPv4 Payload length (host order) >=20 > Ouch, how did this end up being called tot_len... Yeah :/ [snip] > > diff --git a/passt.h b/passt.h > > index 76026f09..d1add470 100644 > > --- a/passt.h > > +++ b/passt.h > > @@ -22,11 +22,11 @@ struct tap_msg { > > /** > > * struct tap_l4_msg - Layer-4 message descriptor for protocol handlers > > * @pkt_buf_offset: Offset of message from @pkt_buf > > - * @l4_len: Length of Layer-4 payload, host order > > + * @l4len: Length of Layer-4 payload, host order >=20 > This is (and was) ambiguous, but in any case, we don't use the struct > anymore since commit bb708111833e ("treewide: Packet abstraction with > mandatory boundary checks"), so I'm fine either way, we don't necessarily > need to drop it here, I think. Huh, hadn't noticed that. I'll insert a patch removing the unused structure before this one. [snip] > > @@ -1658,11 +1660,11 @@ static int tcp_send_flag(struct ctx *c, struct = tcp_tap_conn *conn, int flags) > > th->syn =3D !!(flags & SYN); > > th->fin =3D !!(flags & FIN); > > =20 > > - ip_len =3D tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > > - conn->seq_to_tap); > > - iov[TCP_IOV_PAYLOAD].iov_len =3D ip_len; > > + l4len =3D tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > > + conn->seq_to_tap); > > + iov[TCP_IOV_PAYLOAD].iov_len =3D l4len; > > =20 > > - *(uint32_t *)iov[TCP_IOV_VLEN].iov_base =3D htonl(vnet_len + ip_len); > > + *(uint32_t *)iov[TCP_IOV_VLEN].iov_base =3D htonl(vnet_len + l4len); >=20 > I was completely puzzled by this until I reached 7/7. Okay, this works > because vnet_len includes everything that's not in l4len. Yeah, 'vnet_len' is a bad name here , but fixing that is not within this patch's scope. > There's one bit of confusion left after this series, though. This series > actually highlights that. I'm not sure if it's convenient to fix that > here as well: >=20 > l4len =3D tcp_l2_buf_fill_headers(c, conn, iov, plen, > check, seq); >=20 > This fills in Layer-2 buffers, and returns Layer-3 payload length (i.e. > payload plus size of Layer-4 headers). I didn't really think about it > when the variable was ip_len, but now it's apparent. I'm not entirely clear what fix you're envisaging, and I wonder if it's covered in my subsequent patches. > Further on: >=20 > iov[TCP_IOV_PAYLOAD].iov_len =3D l4len; >=20 > ...so l4len is the length of the payload plus Layer-4 header, but we > use that to set the buffer for TCP_IOV_PAYLOAD... Yeah.. there's some ambiguity in the meaning of "payload". In TCP_IOV_PAYLOAD it means the IP payload, but in 'plen' it means the L4 payload. I'm open to improved names for either "plen" or "TCP_IOV_PAYLOAD". --=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 --q7FfBkf6UVS1CBat Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmYxh0AACgkQzQJF27ox 2GcXzA/+Mrva0eJ2Ep6sLK87nYNYmVobEDzYHEFPbkM+nFQhL2/Mc0VCZ8nr7W2S Y2XKkvIKgvDl/FlmejEAiaUU5pJe0GTvHAHaTYSydcx1tb0Z4P1NrCZLB5w6x+Jc iUl64OEzpfjZa+mVGU+piqLDuHHgmf0/JLlAwZNtR8peu+T72rFDW42aNMbIpmlh X0qJSipifiGh/fb2YoJp9HxM9ApIReJcyPbk2QVLRzlzWKemVcPNHW6YPaaTclHW y0MXByRuH4V9+eqtOZksQ3/7W2eMd9nAM4LFC1nHhS2O4Hdno/t53wSu5Cv5Kfo3 xCwuiLglFUZNU0WQzWdNRonqpDSmlRbhg619BXyTgn1VRNZQWf2kPENVkKZ0SKn/ bfXk/WEio9dkbRXNOBzu7jlPdEVl9UpISM00QwtrBPHrmfIVeN9x12sUbCjgTUU0 Rj1QoUCsPFIekLYpgEokUQqWxMcZxNjJOMMApqTJWox7tdg1yr6y+3+JcBsgZDmI 3kNEq78tBrXVn8tNVpK+oU+hkpvNuw9trvMlLodsLRwBTe9sZocbM5CAEWiN00MX UaEVC9Fs7DHIlnNC8s+RqU9QKcKdJX93mp1vIMl4tgCEMsObzCbIigkZVjHNzcPI VGavGufZThTzMBZa9uR/4MDTDDTnGjYbHWLBZ4DvTkrhnQxnTvM= =IFJD -----END PGP SIGNATURE----- --q7FfBkf6UVS1CBat--