Re: [PATCH v2 3/4] checksum: Add an offset argument in csum_iov()

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 25 Sep 2024 10:11:24 +0200
Laurent Vivier <lvivier@redhat.com> wrote:

> The offset allows any headers are that are not part
> of the data to checksum to be skipped.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
> 
> Notes:
>     v2:
>       - check iov_skip_bytes() return value
> 
>  checksum.c | 16 ++++++++++++++--
>  checksum.h |  3 ++-
>  2 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/checksum.c b/checksum.c
> index 006614fcbb28..68ffaddb5bb0 100644
> --- a/checksum.c
> +++ b/checksum.c
> @@ -59,6 +59,7 @@
>  #include "util.h"
>  #include "ip.h"
>  #include "checksum.h"
> +#include "iov.h"
>  
>  /* Checksums are optional for UDP over IPv4, so we usually just set
>   * them to 0.  Change this to 1 to calculate real UDP over IPv4
> @@ -498,15 +499,26 @@ uint16_t csum(const void *buf, size_t len, uint32_t init)
>   * @iov		Pointer to the array of IO vectors
>   * @n		Length of the array
>   * @init	Initial 32-bit checksum, 0 for no pre-computed checksum
> + * @offset:	Offset of the data to checksum within the full data length

Nit, which I can fix up on merge: @init and @offset are swapped here.

>   *
>   * Return: 16-bit folded, complemented checksum
>   */
>  /* cppcheck-suppress unusedFunction */
> -uint16_t csum_iov(const struct iovec *iov, size_t n, uint32_t init)
> +uint16_t csum_iov(const struct iovec *iov, size_t n, size_t offset,
> +		  uint32_t init)
>  {
>  	unsigned int i;
> +	size_t first;
>  
> -	for (i = 0; i < n; i++)
> +	i = iov_skip_bytes(iov, n, offset, &first);
> +	if (i >= n)
> +		return (uint16_t)~csum_fold(init);
> +
> +	init = csum_unfolded((char *)iov[i].iov_base + first,
> +			     iov[i].iov_len, init);
> +	i++;
> +
> +	for (; i < n; i++)
>  		init = csum_unfolded(iov[i].iov_base, iov[i].iov_len, init);

I wonder if this could become:

	for (i = iov_skip_bytes(iov, n, offset, &start); i < n; i++) {
		init = csum_unfolded((char *)iov[i].iov_base + start,
			 	     iov[i].iov_len, init);
		start = 0;
	}

	return (uint16_t)~csum_fold(init);

...but I haven't tested it and didn't really think it through. I'm fine
with the original version too.

-- 
Stefano