Re: [PATCH] iov: Fix coding style of basic (non-IOV_TAIL) parts

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

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

On Fri, Dec 05, 2025 at 01:51:11AM +0100, Stefano Brivio wrote:
> I happened to touch these functions for a change that turned out to be
> unnecessary, but coding style and documentation could still benefit
> from some rephrasing and fixes.
> 
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

One nit below..

> ---
>  iov.c | 68 +++++++++++++++++++++++++----------------------------------
>  1 file changed, 29 insertions(+), 39 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index 8c74a59..d31464d 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -20,24 +20,21 @@
>   * Contributions after 2012-01-13 are licensed under the terms of the
>   * GNU GPL, version 2 or (at your option) any later version.
>   */
> +
>  #include <sys/socket.h>
>  
>  #include "util.h"
>  #include "iov.h"
>  
> -
>  /**
> - * iov_skip_bytes() - Skip leading bytes of an IO vector
> - * @iov:	IO vector
> + * iov_skip_bytes() - Find index and offset in iovec array given byte offset
> + * @iov:	iovec array
>   * @n:		Number of entries in @iov
> - * @skip:	Number of leading bytes of @iov to skip
> - * @offset:	Offset of first unskipped byte in its @iov entry
> + * @skip:	Byte offset: leading bytes of @iov to skip
> + * @offset:	Offset within matching @iov entry, set on return, can be NULL
>   *
> - * Return: index I of individual struct iovec which contains the byte at @skip
> - *         bytes into the vector (as though all its buffers were contiguous).
> - *         If @offset is non-NULL, update it to the offset of that byte within
> - *         @iov[I] (guaranteed to be less than @iov[I].iov_len) If the whole
> - *         vector has <= @skip bytes, return @n.
> + * Return: index of iovec array containing the @skip byte counted as if buffers
> + *	   were contiguous. If iovec has less than @skip bytes, return @n.
>   */
>  size_t iov_skip_bytes(const struct iovec *iov, size_t n,
>  		      size_t skip, size_t *offset)
> @@ -57,17 +54,14 @@ size_t iov_skip_bytes(const struct iovec *iov, size_t n,
>  }
>  
>  /**
> - * iov_from_buf() - Copy data from a buffer to an I/O vector (struct iovec)
> - *                  efficiently.
> - *
> - * @iov:       Pointer to the array of struct iovec describing the
> - *             scatter/gather I/O vector.
> - * @iov_cnt:   Number of elements in the iov array.
> - * @offset:    Byte offset in the iov array where copying should start.
> - * @buf:       Pointer to the source buffer containing the data to copy.
> - * @bytes:     Total number of bytes to copy from buf to iov.
> + * iov_from_buf() - Copy from flat buffer to iovec array
> + * @iov:	Destination iovec array
> + * @iov_cnt:	Number of elements in the iovec array
> + * @offset:	Destination offset in iovec array

Nit: Any reason not to mention the "counted as if .. contiguous" like
you have for iov_to_buf()?

> + * @buf:	Source buffer
> + * @bytes:	Bytes to copy
>   *
> - * Return: the number of bytes successfully copied.
> + * Return: number of bytes copied
>   */
>  size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>  		    size_t offset, const void *buf, size_t bytes)
> @@ -78,12 +72,12 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>  	if (__builtin_constant_p(bytes) && iov_cnt &&
>  		offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
>  		memcpy((char *)iov[0].iov_base + offset, buf, bytes);
> +
>  		return bytes;
>  	}
>  
>  	i = iov_skip_bytes(iov, iov_cnt, offset, &offset);
>  
> -	/* copying data */
>  	for (copied = 0; copied < bytes && i < iov_cnt; i++) {
>  		size_t len = MIN(iov[i].iov_len - offset, bytes - copied);
>  
> @@ -97,17 +91,14 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt,
>  }
>  
>  /**
> - * iov_to_buf() - Copy data from a scatter/gather I/O vector (struct iovec) to
> - *		  a buffer efficiently.
> - *
> - * @iov:       Pointer to the array of struct iovec describing the scatter/gather
> - *             I/O vector.
> - * @iov_cnt:   Number of elements in the iov array.
> - * @offset:    Offset within the first element of iov from where copying should start.
> - * @buf:       Pointer to the destination buffer where data will be copied.
> - * @bytes:     Total number of bytes to copy from iov to buf.
> + * iov_to_buf() - Copy from iovec to flat buffer
> + * @iov:	Source iovec array
> + * @iov_cnt:	Number of elements in iovec array
> + * @offset:	Source offset in @iov, counted as if buffers were contiguous
> + * @buf:	Destination buffer
> + * @bytes:	Bytes to copy
>   *
> - * Return: the number of bytes successfully copied.
> + * Return: number of bytes copied
>   */
>  size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>  		  size_t offset, void *buf, size_t bytes)
> @@ -118,15 +109,17 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>  	if (__builtin_constant_p(bytes) && iov_cnt &&
>  		offset <= iov[0].iov_len && bytes <= iov[0].iov_len - offset) {
>  		memcpy(buf, (char *)iov[0].iov_base + offset, bytes);
> +
>  		return bytes;
>  	}
>  
>  	i = iov_skip_bytes(iov, iov_cnt, offset, &offset);
>  
> -	/* copying data */
>  	for (copied = 0; copied < bytes && i < iov_cnt; i++) {
>  		size_t len = MIN(iov[i].iov_len - offset, bytes - copied);
> +
>  		ASSERT(iov[i].iov_base);
> +
>  		memcpy((char *)buf + copied, (char *)iov[i].iov_base + offset,
>  		       len);
>  		copied += len;
> @@ -137,14 +130,11 @@ size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt,
>  }
>  
>  /**
> - * iov_size() - Calculate the total size of a scatter/gather I/O vector
> - *              (struct iovec).
> + * iov_size() - Calculate total data size of iovec
> + * @iov:	Source iovec array
> + * @iov_cnt:	Number of elements in iovec array
>   *
> - * @iov:       Pointer to the array of struct iovec describing the
> - *             scatter/gather I/O vector.
> - * @iov_cnt:   Number of elements in the iov array.
> - *
> - * Return: the total size in bytes.
> + * Return: total size in bytes
>   */
>  size_t iov_size(const struct iovec *iov, size_t iov_cnt)
>  {
> -- 
> 2.43.0
> 

-- 
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 --]