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

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Thu, 2015-02-19 at 16:58 +0000, Andrew Cooper wrote:
> 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

How exciting!

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

If only entries of length 0 remain then isn't that a complete-write of
the final non-empty entry?

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

OK.

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

Better safe than sorry then.

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel