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

[Wen Congyang <wency@cn.fujitsu.com>]

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:
> +                    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

> 
> Ian.
> 
> .
>