* [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact()
@ 2014-07-02 18:04 ` Andrew Cooper
2014-07-03 17:15 ` Ian Jackson
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2014-07-02 18:04 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-02 18:04 ` [PATCH] " Andrew Cooper
@ 2014-07-03 17:15 ` Ian Jackson
2014-07-03 17:41 ` Andrew Cooper
2014-07-08 17:10 ` [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact() [and 1 more messages] Ian Jackson
0 siblings, 2 replies; 11+ messages in thread
From: Ian Jackson @ 2014-07-03 17:15 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Ian Campbell, Xen-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-03 17:15 ` Ian Jackson
@ 2014-07-03 17:41 ` Andrew Cooper
2014-07-08 17:10 ` [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact() [and 1 more messages] Ian Jackson
1 sibling, 0 replies; 11+ messages in thread
From: Andrew Cooper @ 2014-07-03 17:41 UTC (permalink / raw)
To: Ian Jackson; +Cc: Ian Campbell, Xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()
@ 2014-07-07 10:18 Andrew Cooper
2014-07-08 3:30 ` Hongyang Yang
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Andrew Cooper @ 2014-07-07 10:18 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Ian Jackson, Ian Campbell
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-07 10:18 [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact() Andrew Cooper
@ 2014-07-08 3:30 ` Hongyang Yang
2014-07-08 9:27 ` David Vrabel
2014-07-08 16:53 ` [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact() Ian Jackson
2 siblings, 0 replies; 11+ messages in thread
From: Hongyang Yang @ 2014-07-08 3:30 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Ian Jackson, Ian Campbell
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-07 10:18 [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact() Andrew Cooper
2014-07-08 3:30 ` Hongyang Yang
@ 2014-07-08 9:27 ` David Vrabel
2014-07-08 9:32 ` Andrew Cooper
2014-07-08 16:53 ` [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact() Ian Jackson
2 siblings, 1 reply; 11+ messages in thread
From: David Vrabel @ 2014-07-08 9:27 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel; +Cc: Ian Jackson, Ian Campbell
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-08 9:27 ` David Vrabel
@ 2014-07-08 9:32 ` Andrew Cooper
2014-07-08 16:58 ` Ian Jackson
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2014-07-08 9:32 UTC (permalink / raw)
To: David Vrabel, Xen-devel; +Cc: Ian Jackson, Ian Campbell
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-07 10:18 [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact() Andrew Cooper
2014-07-08 3:30 ` Hongyang Yang
2014-07-08 9:27 ` David Vrabel
@ 2014-07-08 16:53 ` Ian Jackson
2 siblings, 0 replies; 11+ messages in thread
From: Ian Jackson @ 2014-07-08 16:53 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Ian Campbell, Xen-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-08 9:32 ` Andrew Cooper
@ 2014-07-08 16:58 ` Ian Jackson
2014-07-08 17:00 ` Andrew Cooper
0 siblings, 1 reply; 11+ messages in thread
From: Ian Jackson @ 2014-07-08 16:58 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Ian Campbell, David Vrabel, Xen-devel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact()
2014-07-08 16:58 ` Ian Jackson
@ 2014-07-08 17:00 ` Andrew Cooper
2014-07-02 18:04 ` [PATCH] " Andrew Cooper
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Cooper @ 2014-07-08 17:00 UTC (permalink / raw)
To: Ian Jackson; +Cc: Ian Campbell, David Vrabel, Xen-devel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact() [and 1 more messages]
2014-07-03 17:15 ` Ian Jackson
2014-07-03 17:41 ` Andrew Cooper
@ 2014-07-08 17:10 ` Ian Jackson
1 sibling, 0 replies; 11+ messages in thread
From: Ian Jackson @ 2014-07-08 17:10 UTC (permalink / raw)
To: Andrew Cooper, Xen-devel, Ian Campbell, David Vrabel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-07-08 17:10 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-07 10:18 [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact() Andrew Cooper
2014-07-08 3:30 ` Hongyang Yang
2014-07-08 9:27 ` David Vrabel
2014-07-08 9:32 ` Andrew Cooper
2014-07-08 16:58 ` Ian Jackson
2014-07-08 17:00 ` Andrew Cooper
2014-07-02 18:04 ` [PATCH] " Andrew Cooper
2014-07-03 17:15 ` Ian Jackson
2014-07-03 17:41 ` Andrew Cooper
2014-07-08 17:10 ` [PATCH] tools/libxc: Implement writev_exact() in the same style as write_exact() [and 1 more messages] Ian Jackson
2014-07-08 16:53 ` [PATCH v2] tools/libxc: Implement writev_exact() in the same style as write_exact() Ian Jackson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.