On Tue, Apr 30, 2024 at 08:46:54PM +0200, Stefano Brivio wrote:On Mon, 29 Apr 2024 17:09:29 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:I'd prefer to stick with l3len, so we're consistent across both IPv4 and IPv6. csum_ip4_header() is a special case, becase..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 headerIf we're dealing with IPv4, I guess tot_len is still a reasonable synonym for this, as you left for csum_ip4_header().> l2len: l3len + length of L2 (ethernet) header > > Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> > --- > 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.Yeah :/ [snip]* @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...Huh, hadn't noticed that. I'll insert a patch removing the unused structure before this one. [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 orderThis 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.Yeah, 'vnet_len' is a bad name here , but fixing that is not within this patch's scope.@@ -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.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