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

On Tue, Feb 06, 2024 at 03:28:10PM +0100, Laurent Vivier wrote:
...
  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;
 } 
Right, but done starts at 0 and will remain zero until you reach the
first segment where you need to copy something.  So, unless bytes ==
0, then done < bytes will always be true when offset != 0.  And if
bytes *is* zero, then there's nothing to do, so there's no need to
step through the vector at all.

...
  ...
 
 
  +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. 
Ok, fair enough.

...
  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
  
-- 
David Gibson			| 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