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

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

On 19/02/15 09:49, Ian Campbell wrote:
> On Wed, 2015-02-18 at 18:31 +0000, 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
> s/work/worry?

Oops yes.

>
> Will this still work with non-glibc libcs which do not cope with iovcnt

Yes - observe the min() in the call to writev().  This code never
actually breaks the writev() requirements, but allows the caller of
writev_exact() to be more flexible.

>> IOV_MAX?
> In fact looking at the code it's not clear what glibc is letting work
> anyway, do you pass a count > IOV_MAX to writev? Doesn't look like it.

The page data algorithm in migration v2 submits 1024+7 IOV's at once for
1024 page frames and the associated metadata.  It is specifically useful
not to complicate that function with IOV_MAX adjustments in the case
that IOV_MAX is 1024.

~Andrew

>
>
>> 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>
>> CC: Wei Liu <wei.liu2@citrix.com>
>>
>> ---
>> v4:
>>  * Allow this to compile in a stubdom environment.
>> 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 |   79 ++++++++++++++++++++++++++++++++++++++++++++++
>>  tools/libxc/xc_private.h |   14 ++++++++
>>  2 files changed, 93 insertions(+)
>>
>> diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
>> index df6cd9b..35c514a 100644
>> --- a/tools/libxc/xc_private.c
>> +++ b/tools/libxc/xc_private.c
>> @@ -860,6 +860,85 @@ int write_exact(int fd, const void *data, size_t size)
>>      return 0;
>>  }
>>  
>> +#if defined(__MINIOS__)
>> +/*
>> + * MiniOS's libc doesn't know about writev(). Implement it as multiple write()s.
>> + */
>> +int writev_exact(int fd, const struct iovec *iov, int iovcnt)
>> +{
>> +    int rc, i;
>> +
>> +    for ( i = 0; i < iovcnt; ++i )
>> +    {
>> +        rc = write_exact(fd, iov[i].iov_base, iov[i].iov_len);
>> +        if ( rc )
>> +            return rc;
>> +    }
>> +
>> +    return 0;
>> +}
>> +#else
>> +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;
>> +
>> +        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. */
>> +                if ( !local_iov )
>> +                {
>> +                    local_iov = malloc(iovcnt * sizeof(*iov));
>> +                    if ( !local_iov )
>> +                    {
>> +                        saved_errno = ENOMEM;
>> +                        goto out;
>> +                    }
>> +
>> +                    iov = memcpy(local_iov, iov, iovcnt * sizeof(*iov));
>> +                }
>> +
>> +                local_iov[iov_idx].iov_base += len;
>> +                local_iov[iov_idx].iov_len  -= len;
>> +                break;
>> +            }
>> +        }
>> +    }
>> +
>> +    saved_errno = 0;
>> +
>> + out:
>> +    free(local_iov);
>> +    errno = saved_errno;
>> +    return rc;
>> +}
>> +#endif
>> +
>>  int xc_ffs8(uint8_t x)
>>  {
>>      int i;
>> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
>> index 45b8644..f74f7d7 100644
>> --- a/tools/libxc/xc_private.h
>> +++ b/tools/libxc/xc_private.h
>> @@ -42,6 +42,19 @@
>>  #define VALGRIND_MAKE_MEM_UNDEFINED(addr, len) /* addr, len */
>>  #endif
>>  
>> +#if defined(__MINIOS__)
>> +/*
>> + * MiniOS's libc doesn't know about sys/uio.h or writev().
>> + * Declare enough of sys/uio.h to compile.
>> + */
>> +struct iovec {
>> +    void *iov_base;
>> +    size_t iov_len;
>> +};
>> +#else
>> +#include <sys/uio.h>
>> +#endif
>> +
>>  #define DECLARE_HYPERCALL privcmd_hypercall_t hypercall
>>  #define DECLARE_DOMCTL struct xen_domctl domctl
>>  #define DECLARE_SYSCTL struct xen_sysctl sysctl
>> @@ -395,6 +408,7 @@ int xc_add_mmu_update(xc_interface *xch, struct xc_mmu *mmu,
>>  /* Return 0 on success; -1 on error setting errno. */
>>  int read_exact(int fd, void *data, size_t size); /* EOF => -1, errno=0 */
>>  int write_exact(int fd, const void *data, size_t size);
>> +int writev_exact(int fd, const struct iovec *iov, int iovcnt);
>>  
>>  int xc_ffs8(uint8_t x);
>>  int xc_ffs16(uint16_t x);
>