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

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

On 19/02/15 16:39, Ian Campbell wrote:
> On Thu, 2015-02-19 at 11:45 +0000, Andrew Cooper wrote:
>
>> +    while ( iov_idx < iovcnt )
>> +    {
>> +        /* Skip over iov[] entries with 0 length. */
>> +        while ( iov[iov_idx].iov_len == 0 )
>> +            if ( ++iov_idx == iovcnt )
>> +                goto out;
> Is this required for some reason or just an optimisation?

Experimentally, submitting a writev() with every iov of length 0 results
in an EINVAL on CentOS 5.x

This causes a failure if a partial write adjustment happens and only iov
entries of length 0 remain in the set.

>
>> +
>> +        len = writev(fd, &iov[iov_idx], min(iovcnt - iov_idx, IOV_MAX));
>> +        saved_errno = errno;
>> +
>> +        if ( (len == -1) && (errno == EINTR) )
>> +            continue;
>> +        if ( len <= 0 )
>> +        {
>> +            rc = -1;
>> +            goto out;
>> +        }
>> +
>> +        /* Check iov[] to see whether we had a partial or complete write. */
>> +        while ( len > 0 && (iov_idx < iovcnt) )
>> +        {
>> +            if ( len >= iov[iov_idx].iov_len )
>> +                len -= iov[iov_idx++].iov_len;
>> +            else
>> +            {
>> +                /* Partial write of iov[iov_idx]. Copy iov so we can adjust
>> +                 * element iov_idx and resubmit the rest. */
> I suppose we can't / don't want to just declare that the input is
> non-const and potentially corrupted?

That was the v1 implementation, and specifically objected to during review.

>
>> +                if ( !local_iov )
>> +                {
>> +                    local_iov = malloc(iovcnt * sizeof(*iov));
>> +                    if ( !local_iov )
>> +                    {
>> +                        saved_errno = ENOMEM;
>> +                        goto out;
> What is rc at this point? I think it is 0, but I think you want it to be
> -1?
>
> It might be better to drop the inialiser of rc and set it in the one or
> two places which would then need it (the goto out in the skip-0-length
> loop and just before the out label AFAICT).
>
>

Hmm yes - I shall do.

For what it is worth, I can't find any way of provoking a partial write
of an individual iov element, and I don't realistically expect this
codepath to actually be used.  POSIX however doesn't guarantee that it
can't happen.

~Andrew