Re: [PATCH 01/24] iov: add some functions to manage iovec

On 2/5/24 06:57, David Gibson wrote:
...
  On Fri, Feb 02, 2024 at 03:11:28PM +0100, Laurent
Vivier wrote:
 ...
  diff --git a/iov.c b/iov.c
 new file mode 100644
 index 000000000000..38a8e7566021
 --- /dev/null
 +++ b/iov.c

 +	for (i = 0, done = 0; (offset || done < bytes) && i < iov_cnt; i++) {
 Not immediately seeing why you need the 'offset ||' part of the
condition. 
In fact the loop has two purposes:

1- scan the the iovec to reach byte offset in the iov (so until offset is 0)

2- copy the bytes (until done == byte)

It could be written like this:

for (i = 0; offset && i < iov_cnt && offset >= iov[i].iov_len ;
i++)
         offset -= iov[i].iov_len;

for (done = 0; done < bytes && i < iov_cnt; i++) {
     size_t len = MIN(iov[i].iov_len - offset, bytes - done);
     memcpy((char *)iov[i].iov_base + offset, (char *)buf + done, len);
     done += len;
}

...

...
 
  +unsigned
iov_copy(struct iovec *dst_iov, unsigned int dst_iov_cnt,
 +		  const struct iovec *iov, unsigned int iov_cnt,
 +		  size_t offset, size_t bytes)
 +{
 +	size_t len;
 +	unsigned int i, j;
 +	for (i = 0, j = 0;
 +		 i < iov_cnt && j < dst_iov_cnt && (offset || bytes); i++) {
 +		if (offset >= iov[i].iov_len) {
 +			offset -= iov[i].iov_len;
 +			continue;
 +		}
 +		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;
 +} 
 Small concern about the interface to iov_copy().  If dst_iov_cnt <
 iov_cnt and the chunk of the input iovec you want doesn't fit in the
 destination it will silently truncate - you can't tell if this has
 happened from the return value.  If the assumption is that dst_iov_cnt = iov_cnt,
 then there's not really any need to pass it. 
In fact this function will be removed with "vhost-user: use guest buffer directly in

vu_handle_tx()" as it is not needed when we use directly guest buffers. So I
don't think 
we need to improve this.

If I remember correctly I think it behaves like passt already does with socket backend: if

it doesn't fit, it's dropped silently.

I've fixed all your other comments.

Thanks,
Laurent