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

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

[Andrew Cooper]

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.

Promote the MAX() and MIN() macro definitions from xg_save_restore.h to
xc_private.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

v2: Remove adjustment for partial writes of a specific iov[] entry.
---
 tools/libxc/xc_private.c      |   31 +++++++++++++++++++++++++++++++
 tools/libxc/xc_private.h      |    9 +++++++++
 tools/libxc/xg_save_restore.h |    6 ------
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index a3da614..d610984 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -854,6 +854,37 @@ int write_exact(int fd, const void *data, size_t size)
     return 0;
 }
 
+int writev_exact(int fd, const 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;
+
+        /* Check iov[] to see whether we had a partial or complete write. */
+        while ( len > 0 && (iov_idx < iovcnt) )
+            len -= iov[iov_idx++].iov_len;
+
+        /* writev() guarentees atomicity of individual iov[] elements.  Sanity
+         * check that the returned len did lie on an iov[] element boundary. */
+        assert(len == 0);
+    }
+
+    return 0;
+}
+
 int xc_ffs8(uint8_t x)
 {
     int i;
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 6cc0f2b..198b8a0 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -28,6 +28,7 @@
 #include <sys/stat.h>
 #include <stdlib.h>
 #include <sys/ioctl.h>
+#include <sys/uio.h>
 
 #include "xenctrl.h"
 #include "xenctrlosdep.h"
@@ -343,6 +344,7 @@ int xc_flush_mmu_updates(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);
@@ -367,4 +369,11 @@ int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
 void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
                           uint32_t *port);
 
+#ifndef MAX
+#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
+#endif
+#ifndef MIN
+#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
+#endif
+
 #endif /* __XC_PRIVATE_H__ */
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index aa93c13..34019fa 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -393,9 +393,3 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
         memset(&(_p)->x32._f[0], (_v), sizeof((_p)->x32._f));      \
 } while (0)
 
-#ifndef MAX
-#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
-#endif
-#ifndef MIN
-#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
-#endif
-- 
1.7.10.4

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

[Hongyang Yang]

On 07/07/2014 06:18 PM, 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
> caller is still required to ensure that the sum of iov_len's doesn't overflow
> a ssize_t.
>
> Promote the MAX() and MIN() macro definitions from xg_save_restore.h to
> xc_private.h
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

Reviewed-by: Yang Hongyang <yanghy@cn.fujitsu.com>

>
> ---
>
> v2: Remove adjustment for partial writes of a specific iov[] entry.
> ---
>   tools/libxc/xc_private.c      |   31 +++++++++++++++++++++++++++++++
>   tools/libxc/xc_private.h      |    9 +++++++++
>   tools/libxc/xg_save_restore.h |    6 ------
>   3 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
> index a3da614..d610984 100644
> --- a/tools/libxc/xc_private.c
> +++ b/tools/libxc/xc_private.c
> @@ -854,6 +854,37 @@ int write_exact(int fd, const void *data, size_t size)
>       return 0;
>   }
>
> +int writev_exact(int fd, const 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;
> +
> +        /* Check iov[] to see whether we had a partial or complete write. */
> +        while ( len > 0 && (iov_idx < iovcnt) )
> +            len -= iov[iov_idx++].iov_len;
> +
> +        /* writev() guarentees atomicity of individual iov[] elements.  Sanity
> +         * check that the returned len did lie on an iov[] element boundary. */
> +        assert(len == 0);
> +    }
> +
> +    return 0;
> +}
> +
>   int xc_ffs8(uint8_t x)
>   {
>       int i;
> diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
> index 6cc0f2b..198b8a0 100644
> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
> @@ -28,6 +28,7 @@
>   #include <sys/stat.h>
>   #include <stdlib.h>
>   #include <sys/ioctl.h>
> +#include <sys/uio.h>
>
>   #include "xenctrl.h"
>   #include "xenctrlosdep.h"
> @@ -343,6 +344,7 @@ int xc_flush_mmu_updates(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);
> @@ -367,4 +369,11 @@ int xc_mem_event_memop(xc_interface *xch, domid_t domain_id,
>   void *xc_mem_event_enable(xc_interface *xch, domid_t domain_id, int param,
>                             uint32_t *port);
>
> +#ifndef MAX
> +#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
> +#endif
> +#ifndef MIN
> +#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
> +#endif
> +
>   #endif /* __XC_PRIVATE_H__ */
> diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
> index aa93c13..34019fa 100644
> --- a/tools/libxc/xg_save_restore.h
> +++ b/tools/libxc/xg_save_restore.h
> @@ -393,9 +393,3 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
>           memset(&(_p)->x32._f[0], (_v), sizeof((_p)->x32._f));      \
>   } while (0)
>
> -#ifndef MAX
> -#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
> -#endif
> -#ifndef MIN
> -#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
> -#endif
>

-- 
Thanks,
Yang.

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

[David Vrabel]

On 07/07/14 11:18, 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
> caller is still required to ensure that the sum of iov_len's doesn't overflow
> a ssize_t.
> 
> Promote the MAX() and MIN() macro definitions from xg_save_restore.h to
> xc_private.h
[...]
> --- a/tools/libxc/xc_private.c
> +++ b/tools/libxc/xc_private.c
> @@ -854,6 +854,37 @@ int write_exact(int fd, const void *data, size_t size)
>      return 0;
>  }
>  
> +int writev_exact(int fd, const struct iovec *iov, int iovcnt)
> +{
[...]
> +        /* writev() guarentees atomicity of individual iov[] elements.  Sanity
> +         * check that the returned len did lie on an iov[] element boundary. */
> +        assert(len == 0);

There's nothing in the writev(2) man page that says this.  I think you
need to handle partial writes of an entry.

> --- a/tools/libxc/xc_private.h
> +++ b/tools/libxc/xc_private.h
[...]
> +#ifndef MAX
> +#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
> +#endif
> +#ifndef MIN
> +#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
> +#endif

Do we really want to use these unsafe macros in more places?

David

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

[Andrew Cooper]

On 08/07/14 10:27, David Vrabel wrote:
> On 07/07/14 11:18, 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
>> caller is still required to ensure that the sum of iov_len's doesn't overflow
>> a ssize_t.
>>
>> Promote the MAX() and MIN() macro definitions from xg_save_restore.h to
>> xc_private.h
> [...]
>> --- a/tools/libxc/xc_private.c
>> +++ b/tools/libxc/xc_private.c
>> @@ -854,6 +854,37 @@ int write_exact(int fd, const void *data, size_t size)
>>      return 0;
>>  }
>>  
>> +int writev_exact(int fd, const struct iovec *iov, int iovcnt)
>> +{
> [...]
>> +        /* writev() guarentees atomicity of individual iov[] elements.  Sanity
>> +         * check that the returned len did lie on an iov[] element boundary. */
>> +        assert(len == 0);
> There's nothing in the writev(2) man page that says this.  I think you
> need to handle partial writes of an entry.

Final paragraph in the DESCRIPTION

"The data transfers performed by readv() and writev() are atomic: the
data written by writev() is written as a single block that is not
intermingled with output from writes in other processes"

By my reading, it cannot guarantee atomicity if it would split an iov[]
element.

>
>> --- a/tools/libxc/xc_private.h
>> +++ b/tools/libxc/xc_private.h
> [...]
>> +#ifndef MAX
>> +#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
>> +#endif
>> +#ifndef MIN
>> +#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
>> +#endif
> Do we really want to use these unsafe macros in more places?
>
> David

Perhaphs, perhaps not, but there are no typesafe ones to hand.

~Andrew

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

[Ian Jackson]

Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()"):
> Final paragraph in the DESCRIPTION
> 
> "The data transfers performed by readv() and writev() are atomic: the
> data written by writev() is written as a single block that is not
> intermingled with output from writes in other processes"
> 
> By my reading, it cannot guarantee atomicity if it would split an iov[]
> element.

Firstly, we should be using the spec, not manpages:
 http://pubs.opengroup.org/onlinepubs/9699919799/functions/writev.html
 http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
etc.

There are various guarantees that write and writev provide, including
the special rule for pipes (from write.html):

  Write requests to a pipe or FIFO shall be handled in the same way as
  a regular file with the following exceptions:
     [ complicated rules involving PIPE_BUF ]

and the general rule about system call atomicity:

 http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_07

 All of the following functions shall be atomic with respect to each
 other in the effects specified in POSIX.1-2008 when they operate on
 regular files or symbolic links:
    [...] write() writev() [...]

However neither of these are the guarantee that you are assuming.

In particular, there is nothing saying that _if_ writev performs a
partial write, it won't stop inside an iov.

Ian.

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

[Andrew Cooper]

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.

Promote the MAX() and MIN() macro definitions from xg_save_restore.h to
xc_private.h

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

---

>From strace()ing while writing into a pipe, I have observed quite a few cases
where writev() will perform a partial overall write, but return with len
exactly on an iov[] element boundary.

However, I can't find any guarantee that a partial write won't end midway
through an iov[] element, which is why the adjustment is present.
---
 tools/libxc/xc_private.c      |   34 ++++++++++++++++++++++++++++++++++
 tools/libxc/xc_private.h      |   12 ++++++++++++
 tools/libxc/xg_save_restore.h |    6 ------
 3 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/tools/libxc/xc_private.c b/tools/libxc/xc_private.c
index a3da614..b11d5e5 100644
--- a/tools/libxc/xc_private.c
+++ b/tools/libxc/xc_private.c
@@ -854,6 +854,40 @@ int write_exact(int fd, const void *data, size_t size)
     return 0;
 }
 
+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;
+
+        /* 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--;
+            iov[iov_idx].iov_base += iov[iov_idx].iov_len + len;
+            iov[iov_idx].iov_len = -len;
+        }
+    }
+
+    return 0;
+}
+
 int xc_ffs8(uint8_t x)
 {
     int i;
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index c7730f2..431ab5e 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -28,6 +28,7 @@
 #include <sys/stat.h>
 #include <stdlib.h>
 #include <sys/ioctl.h>
+#include <sys/uio.h>
 
 #include "xenctrl.h"
 #include "xenctrlosdep.h"
@@ -343,6 +344,10 @@ int xc_flush_mmu_updates(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, 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. */
 
 int xc_ffs8(uint8_t x);
 int xc_ffs16(uint16_t x);
@@ -352,4 +357,11 @@ int xc_ffs64(uint64_t x);
 #define DOMPRINTF(fmt, args...) xc_dom_printf(dom->xch, fmt, ## args)
 #define DOMPRINTF_CALLED(xch) xc_dom_printf((xch), "%s: called", __FUNCTION__)
 
+#ifndef MAX
+#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
+#endif
+#ifndef MIN
+#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
+#endif
+
 #endif /* __XC_PRIVATE_H__ */
diff --git a/tools/libxc/xg_save_restore.h b/tools/libxc/xg_save_restore.h
index aa93c13..34019fa 100644
--- a/tools/libxc/xg_save_restore.h
+++ b/tools/libxc/xg_save_restore.h
@@ -393,9 +393,3 @@ static inline int get_platform_info(xc_interface *xch, uint32_t dom,
         memset(&(_p)->x32._f[0], (_v), sizeof((_p)->x32._f));      \
 } while (0)
 
-#ifndef MAX
-#define MAX(_a, _b) ((_a) >= (_b) ? (_a) : (_b))
-#endif
-#ifndef MIN
-#define MIN(_a, _b) ((_a) <= (_b) ? (_a) : (_b))
-#endif
-- 
1.7.10.4

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

[Ian Jackson]

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.

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

Ian.

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

[Andrew Cooper]

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

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

[Andrew Cooper]

On 08/07/14 17:58, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()"):
>> Final paragraph in the DESCRIPTION
>>
>> "The data transfers performed by readv() and writev() are atomic: the
>> data written by writev() is written as a single block that is not
>> intermingled with output from writes in other processes"
>>
>> By my reading, it cannot guarantee atomicity if it would split an iov[]
>> element.
> Firstly, we should be using the spec, not manpages:
>  http://pubs.opengroup.org/onlinepubs/9699919799/functions/writev.html
>  http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html
> etc.
>
> There are various guarantees that write and writev provide, including
> the special rule for pipes (from write.html):
>
>   Write requests to a pipe or FIFO shall be handled in the same way as
>   a regular file with the following exceptions:
>      [ complicated rules involving PIPE_BUF ]
>
> and the general rule about system call atomicity:
>
>  http://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_09_07
>
>  All of the following functions shall be atomic with respect to each
>  other in the effects specified in POSIX.1-2008 when they operate on
>  regular files or symbolic links:
>     [...] write() writev() [...]
>
> However neither of these are the guarantee that you are assuming.
>
> In particular, there is nothing saying that _if_ writev performs a
> partial write, it won't stop inside an iov.
>
> Ian.

Ok - in which case v1 of the patch is correct.  I shall submit v3 which
looks similar to v1, but with some comments improvements

~Andrew

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

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact()"):
> Andrew Cooper writes ("[PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact()"):
> > +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.

Andrew Cooper writes ("Re: [Xen-devel] [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()"):
> Ok - in which case v1 of the patch is correct.  I shall submit v3 which
> looks similar to v1, but with some comments improvements

Please make sure to update the doc comment as I suggest above.

Ian.

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

[Ian Jackson]

Andrew Cooper writes ("[PATCH v2] 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.
...
> +        /* writev() guarentees atomicity of individual iov[] elements.
> +         * Sanity check that the returned len did lie on an iov[]
> +         * element boundary. */
> +        assert(len == 0);

I think you have misunderstood something that I wrote.  writev does
not (in the general case) guarantee not to stop within an iov element.

Ian.