Re: [Patch v3 2/2] tools/libxc: Implement writev_exact() in the same style as write_exact()

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/07/14 11:05, Wen Congyang wrote:
> At 07/18/2014 05:53 PM, Ian Campbell Wrote:
>> On Fri, 2014-07-18 at 10:20 +0100, Andrew Cooper wrote:
>>> On 18/07/14 02:14, Wen Congyang wrote:
>>>> At 07/16/2014 10:32 PM, Andrew Cooper Wrote:
>>>>> This implementation of writev_exact() will cope with an iovcnt greater than
>>>>> IOV_MAX because glibc will actually let this work anyway, and it is very
>>>>> useful not to have to work about this in the caller of writev_exact().  The
>>>>> caller is still required to ensure that the sum of iov_len's doesn't overflow
>>>>> a ssize_t.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>>>>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>>>>>
>>>>> ---
>>>>> v3:
>>>>>  * Re-add adjustment for partial writes.
>>>>>  * Split min/max adjustment into separate patch.
>>>>>
>>>>> v2:
>>>>>  * Remove adjustment for partial writes of a specific iov[] entry.
>>>>> ---
>>>>>  tools/libxc/xc_private.c |   60 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>>  tools/libxc/xc_private.h |    2 ++
>>>>>  2 files changed, 62 insertions(+)
>>>>>
>>>>> diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
>>>>> index 1c214dd..0941b06 100644
>>>>> --- a/tools/libxc/xc_private.c
>>>>> +++ b/tools/libxc/xc_private.c
>>>>> @@ -858,6 +858,66 @@ int write_exact(int fd, const void *data, size_t size)
>>>>>      return 0;
>>>>>  }
>>>>>  
>>>>> +int writev_exact(int fd, const struct iovec *iov, int iovcnt)
>>>>> +{
>>>>> +    struct iovec *local_iov = NULL;
>>>>> +    int rc = 0, iov_idx = 0, saved_errno = 0;
>>>>> +    ssize_t len;
>>>>> +
>>>>> +    while ( iov_idx < iovcnt )
>>>>> +    {
>>>>> +        /* Skip over iov[] entries with 0 length. */
>>>>> +        while ( iov[iov_idx].iov_len == 0 )
>>>>> +            if ( ++iov_idx == iovcnt )
>>>>> +                goto out;
>>>> set saved_errn to 0 before goto out?
>>> Good catch.
>> Isn't this a success path? errno is generally undefined on success.
> Yes, but we set saved_errno to 0 here:
>> +    saved_errno = 0;
>> +
>> + out:
>> +    free(local_iov);
>> +    errno = saved_errno;
>> +    return rc;
>> +}
> I think there is no need to save errno in this function, because
> we return -1 when writev()/malloc() fails.
>
> Another problem:

I am deliberately creating the same (somewhat quirky) error semantics of
write_exact(), for consistency reasons.

The key point is that EOF is -1 and errno of 0.

>> +                    local_iov = malloc(iovcnt * sizeof(*iov));
>> +                    if ( !local_iov )
>> +                    {
>> +                        saved_errno = ENOMEM;
>> +                        goto out;
>> +                    }
>> +
> rc is not set to -1 before goto out.
>
> Thanks
> Wen Congyang

Good point.

~Andrew