Re: [PATCH v4 5/8] checksum: use csum_ip4_header() in udp.c and tcp.c

On 3/1/24 00:25, David Gibson wrote:
...
  On Thu, Feb 29, 2024 at 05:59:52PM +0100, Laurent
Vivier wrote:
  We can find the same function to compute the IPv4
header
 checksum in tcp.c, udp.c and tap.c

 Use the function defined for tap.c, csum_ip4_header(), but
 with the code used in tcp.c and udp.c as it doesn't need a fully
 initialiazed IPv4 header, only protocol, tot_len, saddr and daddr.

 Signed-off-by: Laurent Vivier <lvivier(a)redhat.com>
 Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au>
 ---

 Notes:
      v4:
        - rebase
      
      v3:
        - function parameters provide tot_len, saddr, daddr and protocol
         rather than an iphdr
      
      v2:
        - use csum_ip4_header() from checksum.c
        - use code from tcp.c and udp.c in csum_ip4_header()
        - use "const struct iphfr *", check is not updated by the
          function but by the caller.

   checksum.c | 17 +++++++++++++----
   checksum.h |  3 ++-
   tap.c      |  3 ++-
   tcp.c      | 24 +++---------------------
   udp.c      | 20 ++------------------
   5 files changed, 22 insertions(+), 45 deletions(-)

 diff --git a/checksum.c b/checksum.c
 index 74e3742bc6f6..511b296a9a80 100644
 --- a/checksum.c
 +++ b/checksum.c
 @@ -57,6 +57,7 @@
   #include <linux/icmpv6.h>
   
   #include "util.h"
 +#include "ip.h"
   #include "checksum.h"
   
   /* Checksums are optional for UDP over IPv4, so we usually just set
 @@ -116,13 +117,21 @@ uint16_t csum_fold(uint32_t sum)
   uint16_t csum(const void *buf, size_t len, uint32_t init);
   
   /**
 - * csum_ip4_header() - Calculate and set IPv4 header checksum
 + * csum_ip4_header() - Calculate IPv4 header checksum
    * @ip4h:	IPv4 header 
 
 Function comment needs to be updated for the new parameters.  In
 particular it needs to note that tot_len, saddr and daddr are all
 passed in network order.
 
 As noted elsewhere, I kind of hate passing non-host-endian values in
 plain integer types, but I can see why doing otherwise here would be
 very awkward. 
Perhaps we can use __be16 and __be32 types to really show the endianness in the code?

Thanks,
Laurent