Re: [PATCH 02/18] iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 13921 bytes --]

On Wed, Apr 02, 2025 at 07:23:27PM +0200, Laurent Vivier wrote:
> Provide a temporary variable of the wanted type to store
> the header if the memory in the iovec array is not contiguous.
> 
> Also introduce iov_tail_ptr(), iov_drop() and iov_copy()
> for later use.

Not sure if these belong in the same patch.

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  iov.c     | 133 ++++++++++++++++++++++++++++++++++++++++++++++++++----
>  iov.h     |  53 ++++++++++++++++------
>  tcp_buf.c |   2 +-
>  3 files changed, 163 insertions(+), 25 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index 8c63b7ea6e31..d96fc2ab594b 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -108,7 +108,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>   *
>   * Returns:    The number of bytes successfully copied.
>   */
> -/* cppcheck-suppress unusedFunction */
> +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */
>  size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>  		  size_t offset, void *buf, size_t bytes)
>  {
> @@ -126,6 +126,7 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>  	/* copying data */
>  	for (copied = 0; copied < bytes && i < iov_cnt; i++) {
>  		size_t len = MIN(iov[i].iov_len - offset, bytes - copied);
> +		/* NOLINTNEXTLINE(clang-analyzer-core.NonNullParamChecker) */
>  		memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset,
>  		       len);
>  		copied += len;
> @@ -156,6 +157,45 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt)
>  	return len;
>  }
>  
> +/**
> + * iov_copy - Copy data from one scatter/gather I/O vector (struct iovec) to
> + *	      another.

I think this function name is slightly misleading and the description
is very misleading.  What the function does is make a new iov
referencing a subset of the data in the original iov.  It doesn't copy
the reference data - the dst iov will alias (some of) the same memory
as the src iov.

I'd suggest iov_slice() maybe?

> + * @dst_iov:     Pointer to the destination array of struct iovec describing
> + *		 the scatter/gather I/O vector to copy to.
> + * @dst_iov_cnt: Number of elements in the destination iov array.

Maybe worth emphasising that this is a maximum, we might not use all of them.

> + * @iov:	 Pointer to the source array of struct iovec describing
> + *		 the scatter/gather I/O vector to copy from.
> + * @iov_cnt:     Number of elements in the source iov array.
> + * @offset:      Offset within the source iov from where copying should start.
> + * @bytes:	 Total number of bytes to copy from iov to dst_iov.
> + *
> + * Returns:      The number of elements successfully copied to the destination
> + *		 iov array.

So, this is obviously useful, because it's effectively the "after"
value of dst_iov.  However, it doesn't necessarily distinguish the
(AFAICT) three exit conditions:
  1) "success"; iov_size(dst_iov) == bytes afterwards
  2) iov_size(dst_iov) < bytes, because we ran out of data in the
     source iov
  3) iov_size(dst_iov) < bytes, because we needed more than
     @dst_iov_cnt "slots" to reference that much of @iov.

On "failure", whether it's (2) or (3) seems like it could matter, so
it would be nice if we can deduce that from the return value.

> + */
> +/* cppcheck-suppress unusedFunction */
> +unsigned iov_copy(struct iovec *dst_iov, size_t dst_iov_cnt,
> +		  const struct iovec *iov, size_t iov_cnt,
> +		  size_t offset, size_t bytes)
> +{
> +	unsigned int i, j;
> +
> +	i = iov_skip_bytes(iov, iov_cnt, offset, &offset);
> +
> +	/* copying data */
> +	for (j = 0; i < iov_cnt && j < dst_iov_cnt && bytes; i++) {
> +		size_t len = MIN(bytes, iov[i].iov_len - offset);
> +
> +		dst_iov[j].iov_base = (char *)iov[i].iov_base + offset;
> +		dst_iov[j].iov_len = len;
> +		j++;
> +		bytes -= len;
> +		offset = 0;
> +	}
> +
> +	return j;
> +}
> +
>  /**
>   * iov_tail_prune() - Remove any unneeded buffers from an IOV tail
>   * @tail:	IO vector tail (modified)
> @@ -192,7 +232,50 @@ size_t iov_tail_size(struct iov_tail *tail)
>  }
>  
>  /**
> - * iov_peek_header_() - Get pointer to a header from an IOV tail
> + * iov_drop() - update head of the tail

Cute phrasing, but kind of confusing.  I think this wants similar
phrasing to remove_header, since it's doing basically the same thing,
but discarding rather than retreiving the data.

> + * @tail:	IO vector tail
> + * @len:	length to move the head of the tail
> + *
> + * Returns:	true if the tail still contains any bytes, otherwise false
> + */
> +/* cppcheck-suppress unusedFunction */
> +bool iov_drop(struct iov_tail *tail, size_t len)
> +{
> +	tail->off = tail->off + len;
> +
> +	return iov_tail_prune(tail);
> +}
> +
> +/**
> + * iov_tail_ptr() - get a pointer to the head of the tail
> + * @tail:	IO vector tail
> + * @off:	Byte offset in the tail to skip
> + * @len:	Length of the data to get, in bytes
> + *
> + * Returns:	pointer to the data in the tail, NULL if the
> + *		tail doesn't contains @len bytes.

This is a dangerous interface, because it returns a pointer with no
indication of how many bytes after it are actually valid.  I can see
it being useful internally, but I don't think it should be exported.

> + */
> +/* cppcheck-suppress unusedFunction */
> +void *iov_tail_ptr(struct iov_tail *tail, size_t off, size_t len)
> +{
> +	const struct iovec *iov;
> +	size_t cnt, i;
> +
> +	i = iov_skip_bytes(tail->iov, tail->cnt, tail->off + off, &off);
> +	if (i == tail->cnt)
> +		return NULL;
> +
> +	iov = &tail->iov[i];
> +	cnt = tail->cnt - i;
> +
> +	if (iov_size(iov, cnt) < off + len)
> +		return NULL;
> +
> +	return (char *)iov[0].iov_base + off;
> +}
> +
> +/**
> + * iov_check_header - Check if a header can be accessed
>   * @tail:	IOV tail to get header from
>   * @len:	Length of header to get, in bytes
>   * @align:	Required alignment of header, in bytes
> @@ -203,8 +286,7 @@ size_t iov_tail_size(struct iov_tail *tail)
>   *	    overruns the IO vector, is not contiguous or doesn't have the
>   *	    requested alignment.
>   */
> -/* cppcheck-suppress [staticFunction,unmatchedSuppression] */
> -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
> +static void *iov_check_header(struct iov_tail *tail, size_t len, size_t align)
>  {
>  	char *p;
>  
> @@ -225,7 +307,36 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
>  }
>  
>  /**
> - * iov_remove_header_() - Remove a header from an IOV tail
> + * iov_peek_header_ - Get pointer to a header from an IOV tail
> + * @tail:	IOV tail to get header from

Needs a description for @v.

> + * @len:	Length of header to get, in bytes
> + * @align:	Required alignment of header, in bytes
> + *
> + * @tail may be pruned, but will represent the same bytes as before.
> + *
> + * Returns: Pointer to the first @len logical bytes of the tail, or to
> + *          a copy if that overruns the IO vector, is not contiguous or
> + *          doesn't have the requested alignment. NULL if that overruns the
> + *          IO vector.
> + */
> +/* cppcheck-suppress [staticFunction,unmatchedSuppression] */
> +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
> +{
> +	char *p = iov_check_header(tail, len, align);
> +	size_t l;
> +
> +	if (p)
> +		return p;
> +
> +	l = iov_to_buf(tail->iov, tail->cnt,  tail->off, v, len);
> +	if (l != len)
> +		return NULL;
> +
> +	return v;
> +}
> +
> +/**
> + * iov_remove_header_ - Remove a header from an IOV tail
>   * @tail:	IOV tail to remove header from (modified)

Needs description of @v.

>   * @len:	Length of header to remove, in bytes
>   * @align:	Required alignment of header, in bytes
> @@ -233,17 +344,19 @@ void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align)
>   * On success, @tail is updated so that it longer includes the bytes of the
>   * returned header.
>   *
> - * Returns: Pointer to the first @len logical bytes of the tail, NULL if that
> - *	    overruns the IO vector, is not contiguous or doesn't have the
> - *	    requested alignment.
> + * Returns: Pointer to the first @len logical bytes of the tail, or to
> + *          a copy if that overruns the IO vector, is not contiguous or
> + *          doesn't have the requested alignment. NULL if that overruns the
> + *          IO vector.
>   */
> -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align)
> +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align)
>  {
> -	char *p = iov_peek_header_(tail, len, align);
> +	char *p = iov_peek_header_(tail, v, len, align);
>  
>  	if (!p)
>  		return NULL;
>  
>  	tail->off = tail->off + len;
> +
>  	return p;
>  }
> diff --git a/iov.h b/iov.h
> index 9855bf0c0c32..f9b5fdf0803e 100644
> --- a/iov.h
> +++ b/iov.h
> @@ -28,6 +28,9 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>  size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>                    size_t offset, void *buf, size_t bytes);
>  size_t iov_size(const struct iovec *iov, size_t iov_cnt);
> +unsigned iov_copy(struct iovec *dst_iov, size_t dst_iov_cnt,
> +                  const struct iovec *iov, size_t iov_cnt,
> +                  size_t offset, size_t bytes);
>  
>  /*
>   * DOC: Theory of Operation, struct iov_tail
> @@ -70,38 +73,60 @@ struct iov_tail {
>  #define IOV_TAIL(iov_, cnt_, off_) \
>  	(struct iov_tail){ .iov = (iov_), .cnt = (cnt_), .off = (off_) }
>  
> +/**
> + * IOV_TAIL_FROM_BUF() - Create a new IOV tail from a buffer
> + * @buf_:	Buffer address to use in the iovec
> + * @len_:	Buffer size
> + * @off_:	Byte offset in the buffer where the tail begins
> + */
> +#define IOV_TAIL_FROM_BUF(buf_, len_, off_) \
> +	IOV_TAIL((&(const struct iovec){ .iov_base = (buf_), .iov_len = (len_) }), 1, (off_))
> +
>  bool iov_tail_prune(struct iov_tail *tail);
>  size_t iov_tail_size(struct iov_tail *tail);
> -void *iov_peek_header_(struct iov_tail *tail, size_t len, size_t align);
> -void *iov_remove_header_(struct iov_tail *tail, size_t len, size_t align);
> +bool iov_drop(struct iov_tail *tail, size_t len);
> +void *iov_peek_header_(struct iov_tail *tail, void *v, size_t len, size_t align);
> +void *iov_remove_header_(struct iov_tail *tail, void *v, size_t len, size_t align);
> +void *iov_tail_ptr(struct iov_tail *tail, size_t off, size_t len);
>  
>  /**
>   * IOV_PEEK_HEADER() - Get typed pointer to a header from an IOV tail
>   * @tail_:	IOV tail to get header from
> - * @type_:	Data type of the header
> + * @var_:	Temporary buffer of the type of the header to use if
> + *		the memory in the iovec array is not contiguous.
>   *
>   * @tail_ may be pruned, but will represent the same bytes as before.
>   *
> - * Returns: Pointer of type (@type_ *) located at the start of @tail_, NULL if
> - *          we can't get a contiguous and aligned pointer.
> + * Returns: Pointer of type (@type_ *) located at the start of @tail_

This is no longer quite accurate: the data is the same, but it's not
necessarily a pointer into @tail_ proper.  There's also still a NULL
return case, just more limited than it was before.

>   */
> -#define IOV_PEEK_HEADER(tail_, type_)					\
> -	((type_ *)(iov_peek_header_((tail_),				\
> -				    sizeof(type_), __alignof__(type_))))
> +
> +#define IOV_PEEK_HEADER(tail_, var_)					\
> +	((__typeof__(var_) *)(iov_peek_header_((tail_), &(var_),	\
> +				      sizeof(var_), __alignof__(var_))))
>  
>  /**
>   * IOV_REMOVE_HEADER() - Remove and return typed header from an IOV tail
>   * @tail_:	IOV tail to remove header from (modified)
> - * @type_:	Data type of the header to remove
> + * @var_:	Temporary buffer of the type of the header to use if
> + *		the memory in the iovec array is not contiguous.
>   *
>   * On success, @tail_ is updated so that it longer includes the bytes of the
>   * returned header.
>   *
> - * Returns: Pointer of type (@type_ *) located at the old start of @tail_, NULL
> - *          if we can't get a contiguous and aligned pointer.
> + * Returns: Pointer of type (@type_ *) located at the old start of @tail_
> + */
> +
> +#define IOV_REMOVE_HEADER(tail_, var_)					\
> +	((__typeof__(var_) *)(iov_remove_header_((tail_), &(var_),	\
> +				    sizeof(var_), __alignof__(var_))))
> +
> +/** IOV_DROP_HEADER() -- Remove a typed header from an IOV tail
> + * @tail_:	IOV tail to remove header from (modified)
> + * @type_:	Data type of the header to remove
> + *
> + * Returns:	true if the tail still contains any bytes, otherwise false
>   */
> -#define IOV_REMOVE_HEADER(tail_, type_)					\
> -	((type_ *)(iov_remove_header_((tail_),				\
> -				      sizeof(type_), __alignof__(type_))))
> +#define IOV_DROP_HEADER(tail_, type_)					\
> +	iov_drop((tail_), sizeof(type_))
>  
>  #endif /* IOVEC_H */
> diff --git a/tcp_buf.c b/tcp_buf.c
> index 05305636b503..4bcc1acb245a 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -160,7 +160,7 @@ static void tcp_l2_buf_fill_headers(const struct tcp_tap_conn *conn,
>  				    uint32_t seq, bool no_tcp_csum)
>  {
>  	struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0);
> -	struct tcphdr *th = IOV_REMOVE_HEADER(&tail, struct tcphdr);
> +	struct tcphdr thc, *th = IOV_REMOVE_HEADER(&tail, thc);
>  	struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base;
>  	const struct flowside *tapside = TAPFLOW(conn);
>  	const struct in_addr *a4 = inany_v4(&tapside->oaddr);

-- 
David Gibson (he or they)	| 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]