Re: [PATCH 1/5] packet: replace struct desc by struct iovec

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

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

On Fri, Jun 21, 2024 at 04:56:36PM +0200, Laurent Vivier wrote:

Needs a commit message.

> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  packet.c | 75 +++++++++++++++++++++++++++++++-------------------------
>  packet.h | 14 ++---------
>  2 files changed, 43 insertions(+), 46 deletions(-)
> 
> diff --git a/packet.c b/packet.c
> index ccfc84607709..af2a539a1794 100644
> --- a/packet.c
> +++ b/packet.c
> @@ -22,6 +22,36 @@
>  #include "util.h"
>  #include "log.h"
>

Function comment, please.

> +static int packet_check_range(const struct pool *p, size_t offset, size_t len,
> +			      const char *start, const char *func, int line)
> +{
> +	if (start < p->buf) {
> +		if (func) {
> +			trace("add packet start %p before buffer start %p, "
> +			      "%s:%i", (void *)start, (void *)p->buf, func, line);
> +		}
> +		return -1;

Pre-existing, but I wonder if these should be assert()s.  Are there
any cases where we'd hit this path that don't indicate a bug in the
caller?

> +	}
> +
> +	if (start + len + offset > p->buf + p->buf_size) {

Also pre-existing, but I wonder if we should check for overflow of
(Start + len + offset).

> +		if (func) {
> +			trace("packet offset plus length %lu from size %lu, "
> +			      "%s:%i", start - p->buf + len + offset,
> +			      p->buf_size, func, line);
> +		}
> +		return -1;
> +	}
> +
> +#if UINTPTR_MAX == UINT64_MAX
> +	if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) {

I don't think this check is relevant any more if we're going to iovecs
- this was just because the offset in struct desc was only 32-bit.

> +		trace("add packet start %p, buffer start %p, %s:%i",
> +		      (void *)start, (void *)p->buf, func, line);
> +		return -1;
> +	}
> +#endif
> +
> +	return 0;
> +}
>  /**
>   * packet_add_do() - Add data as packet descriptor to given pool
>   * @p:		Existing pool
> @@ -41,34 +71,16 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
>  		return;
>  	}
>  
> -	if (start < p->buf) {
> -		trace("add packet start %p before buffer start %p, %s:%i",
> -		      (void *)start, (void *)p->buf, func, line);
> +	if (packet_check_range(p, 0, len, start, func, line))
>  		return;
> -	}
> -
> -	if (start + len > p->buf + p->buf_size) {
> -		trace("add packet start %p, length: %zu, buffer end %p, %s:%i",
> -		      (void *)start, len, (void *)(p->buf + p->buf_size),
> -		      func, line);
> -		return;
> -	}
>  
>  	if (len > UINT16_MAX) {
>  		trace("add packet length %zu, %s:%i", len, func, line);
>  		return;
>  	}
>  
> -#if UINTPTR_MAX == UINT64_MAX
> -	if ((uintptr_t)start - (uintptr_t)p->buf > UINT32_MAX) {
> -		trace("add packet start %p, buffer start %p, %s:%i",
> -		      (void *)start, (void *)p->buf, func, line);
> -		return;
> -	}
> -#endif
> -
> -	p->pkt[idx].offset = start - p->buf;
> -	p->pkt[idx].len = len;
> +	p->pkt[idx].iov_base = (void *)start;
> +	p->pkt[idx].iov_len = len;
>  
>  	p->count++;
>  }
> @@ -104,28 +116,23 @@ void *packet_get_do(const struct pool *p, size_t idx, size_t offset,
>  		return NULL;
>  	}
>  
> -	if (p->pkt[idx].offset + len + offset > p->buf_size) {
> +	if (len + offset > p->pkt[idx].iov_len) {
>  		if (func) {
> -			trace("packet offset plus length %zu from size %zu, "
> -			      "%s:%i", p->pkt[idx].offset + len + offset,
> -			      p->buf_size, func, line);
> +			trace("data length %zu, offset %zu from length %zu, "
> +			      "%s:%i", len, offset, p->pkt[idx].iov_len,
> +			      func, line);

I'm not sure either the old or new message is particularly descriptive
here :/

>  		}
>  		return NULL;
>  	}
>  
> -	if (len + offset > p->pkt[idx].len) {
> -		if (func) {
> -			trace("data length %zu, offset %zu from length %u, "
> -			      "%s:%i", len, offset, p->pkt[idx].len,
> -			      func, line);
> -		}
> +	if (packet_check_range(p, offset, len, p->pkt[idx].iov_base,
> +			       func, line))

Ah.. right..  in this case we certainly don't want ASSERT()s in
packet_check_range().  Still wonder if that would make more sense for
the packet add case, however.

A couple of other points:
 * You've effectively switched the order of the two different tests here
   (one range checking against the entire buffer, one range checking
   against a single packet).  Any reason for that?
 * Do we actually need the entire-buffer check here on the _get()
   side?  Isn't it enough to ensure that packets lie within the buffer
   when they're inserted?  Pre-existing, again, AFAICT.

>  		return NULL;
> -	}
>  
>  	if (left)
> -		*left = p->pkt[idx].len - offset - len;
> +		*left = p->pkt[idx].iov_len - offset - len;
>  
> -	return p->buf + p->pkt[idx].offset + offset;
> +	return (char *)p->pkt[idx].iov_base + offset;
>  }
>  
>  /**
> diff --git a/packet.h b/packet.h
> index a784b07bbed5..8377dcf678bb 100644
> --- a/packet.h
> +++ b/packet.h
> @@ -6,16 +6,6 @@
>  #ifndef PACKET_H
>  #define PACKET_H
>  
> -/**
> - * struct desc - Generic offset-based descriptor within buffer
> - * @offset:	Offset of descriptor relative to buffer start, 32-bit limit
> - * @len:	Length of descriptor, host order, 16-bit limit
> - */
> -struct desc {
> -	uint32_t offset;
> -	uint16_t len;
> -};
> -
>  /**
>   * struct pool - Generic pool of packets stored in a buffer
>   * @buf:	Buffer storing packet descriptors
> @@ -29,7 +19,7 @@ struct pool {
>  	size_t buf_size;
>  	size_t size;
>  	size_t count;
> -	struct desc pkt[1];
> +	struct iovec pkt[1];
>  };
>  
>  void packet_add_do(struct pool *p, size_t len, const char *start,
> @@ -54,7 +44,7 @@ struct _name ## _t {							\
>  	size_t buf_size;						\
>  	size_t size;							\
>  	size_t count;							\
> -	struct desc pkt[_size];						\
> +	struct iovec pkt[_size];					\
>  }
>  
>  #define PACKET_POOL_INIT_NOCAST(_size, _buf, _buf_size)			\

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