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

[Laurent Vivier <lvivier@redhat.com>]

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