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

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

On 03/07/2014 18:15, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact()"):
>> 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.
> ...
>> +int writev_exact(int fd, struct iovec *iov, int iovcnt)
>> +{
>> +    int iov_idx = 0;
>> +    ssize_t len;
>> +
>> +    while ( iov_idx < iovcnt )
>> +    {
>> +        /* Skip over iov[] enties with 0 length. */
>> +        while ( iov[iov_idx].iov_len == 0 )
>> +            if ( ++iov_idx == iovcnt )
>> +                return 0;
>> +
>> +        len = writev(fd, &iov[iov_idx], MIN(iovcnt - iov_idx, IOV_MAX));
>> +
>> +        if ( (len == -1) && (errno == EINTR) )
>> +            continue;
>> +        if ( len <= 0 )
>> +            return -1;
> If writev does return 0, you at the very least need to set errno
> before changing the return value to -1.

Hmm - that should probably be "return len", to have the same EOF 
behaviour as write_exact().  In that case, errno should be correct 
without further modification.

Errno does however need to be set for the previous return 0.

>
>> +        /* Check iov[] to see whether we had a partial or complete write. */
>> +        while ( len > 0 && (iov_idx < iovcnt) )
>> +            len -= iov[iov_idx++].iov_len;
>> +
>> +        if ( len < 0 ) /* Partial write of iov[iov_idx - 1]. */
>> +        {
>> +            iov_idx--;
> This logic is rather wtf!  Isn't there a way of expressing it that
> doesn't involve len becoming negative and unwinding iov_idx ?
>
>> +int writev_exact(int fd, struct iovec *iov, int iovcnt);
>> +/* Note - writev_exact() might modify iov.  Whether it does so in practice
>> + * depends on whether your system implementation of writev() returns from a
>> + * partial write in the middle of an iov element. */
> The second sentence should be removed.  No-one is allowed to assume
> that writev doesn't do so.  Also, you should mention that your
> writev_exact lacks the atomicity guarantee of proper writev.

Actually I wonder.

Rethinking this somewhat, the atomicity guarentee of the proper writev() 
wrt individual iov[] elements constitute a guarentee that it shall never 
return having performed a partial write of an individual iov[] element.  
I came to this conclusion first, then somehow argued myself out of it.  
Can I have a second opinion?

~Andrew