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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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