Re: [PATCH] util: fix confusion between offset in the iovec array and in the entry

[Laurent Vivier <lvivier@redhat.com>]

On 3/20/24 05:27, David Gibson wrote:
> On Tue, Mar 19, 2024 at 11:13:40AM +0100, Laurent Vivier wrote:
>> In write_remainder() 'skip' is the offset to start the operation from
>> in the iovec array.
>>
>> In iov_skip_bytes(), 'skip' is also the offset in the iovec array but
>> 'offset' is the first unskipped byte in the iovec entry.
>>
>> As write_remainder() uses 'skip' for both, 'skip' is reset to the
>> first unskipped byte in the iovec entry rather to staying the first
>> unskipped byte in the iovec array.
>>
>> Fix the problem by introducing a new variable not to overwrite 'skip'
>> on each loop.
>>
>> Fixes: 8bdb0883b441 ("util: Add write_remainder() helper")
>> Cc: david@gibson.dropbear.id.au
>> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> 
> Ouch.  *dons paper bag*
> 
> I believe this is correct as it stands, but..
> 
>> ---
>>   util.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/util.c b/util.c
>> index 3b2393d6bfa8..eee53aed811b 100644
>> --- a/util.c
>> +++ b/util.c
>> @@ -533,13 +533,14 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
>>   int write_remainder(int fd, const struct iovec *iov, int iovcnt, size_t skip)
>>   {
>>   	int i;
>> +	size_t offset;
>>   
>> -	while ((i = iov_skip_bytes(iov, iovcnt, skip, &skip)) < iovcnt) {
>> +	while ((i = iov_skip_bytes(iov, iovcnt, skip, &offset)) < iovcnt) {
>>   		ssize_t rc;
>>   
>>   		if (skip) {
> 
> .. it would be more optimal if you checked for offset != 0 rather than
> skip != 0 here.  Otherwise even once we've written an entire buffer,
> we'll continue to write the rest of the buffers one by one, rather
> than writing them all with a writev().
> 
Yes, you're right. I missed that.

Thanks,
Laurent