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: > > > 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 same > > 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. > > > > 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 > > 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.. > > > l2len: l3len + length of L2 (ethernet) header > > > > 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(-) > > > > 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) > > > > /** > > * 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) ..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) > > 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 > > 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 = !!(flags & SYN); > > th->fin = !!(flags & FIN); > > > > - ip_len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > > - conn->seq_to_tap); > > - iov[TCP_IOV_PAYLOAD].iov_len = ip_len; > > + l4len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > > + conn->seq_to_tap); > > + iov[TCP_IOV_PAYLOAD].iov_len = l4len; > > > > - *(uint32_t *)iov[TCP_IOV_VLEN].iov_base = htonl(vnet_len + ip_len); > > + *(uint32_t *)iov[TCP_IOV_VLEN].iov_base = htonl(vnet_len + l4len); > > 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: > > l4len = tcp_l2_buf_fill_headers(c, conn, iov, plen, > check, seq); > > 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: > > iov[TCP_IOV_PAYLOAD].iov_len = l4len; > > ...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". -- 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