[Libvir] take 2 [Re: write(2) may write less than the total requested
Daniel Veillard [EMAIL PROTECTED] wrote:
On Wed, Feb 20, 2008 at 02:53:49PM +0100, Jim Meyering wrote:
Use safewrite in place of write, in many cases.
And add make syntax-check rules to ensure no new uses sneak in.
There are many uses of write like this:
if (write (fd, xml, towrite) != towrite)
return -1;
The problem is that the syscall can succeed, yet write less than
the requested number of bytes, so the caller should retry
rather than simply failing.
agreed using safewrite is a good idea, +1
This patch changes most of them to use util.c's safewrite wrapper,
which encapsulates the process. Also, there were a few cases in
which the retry loop was open-coded, and I replaced those, too.
However, there remain invalid uses of write in both virsh.c and
console.c. I would have replaced those, too, but util.c isn't
linked with them, so it would have made this change more involved.
xend_internal.c has one use, but it's ok, and not easily replaced.
maybe for case outside of the library adding a copy of safewrite
in virsh.c would be a good way to do this while keeping the code
consistent. The duplication of that small function whould not
be a real problem IMHO
Thanks for the quick review.
That would work well.
It's needed also for qemud.c, which already includes util.h
and uses functions like __virStrToLong_i, yet linking libvirtd
would currently fail due to the uses of safewrite.
If I do as you suggest and move the safewrite definition into util.h,
and make it static inline, then both problems go away.
Here's a new version of the patch, adjusted accordingly:
---
Use safewrite in place of write, in many cases.
And add make syntax-check rules to ensure no new uses sneak in.
There are many uses of write like this:
if (write (fd, xml, towrite) != towrite)
return -1;
The problem is that the syscall can succeed, yet write less than
the requested number of bytes, so the caller should retry
rather than simply failing.
This patch changes most of them to use util.c's safewrite wrapper,
which encapsulates the process. Also, there were a few cases in
which the retry loop was open-coded, and I replaced those, too.
However, there remain invalid uses of write in both virsh.c and
console.c. I would have replaced those, too, but util.c isn't
linked with them, so it would have made this change more involved.
xend_internal.c has one use, but it's ok, and not easily replaced.
Note: the change to libvirt_proxy.c removes a diagnostic that
would have warned about an EINTR-interrupted (and retried) write,
but that seems ok.
* Makefile.maint (sc_avoid_write): New rule, to avoid recurrence.
* .x-sc_avoid_write: New file. Record two legitimate exemptions.
* qemud/qemud.c (sig_handler, qemudClientWriteBuf): Use safewrite, not write.
* src/conf.c (__virConfWriteFile): Likewise.
* src/qemu_conf.c (qemudSaveConfig, qemudSaveNetworkConfig): Likewise.
* src/qemu_driver.c (qemudWaitForMonitor, qemudStartVMDaemon)
(qemudVMData, PROC_IP_FORWARD): Likewise.
* proxy/libvirt_proxy.c: Include util.h.
(proxyWriteClientSocket): Use safewrite.
* src/test.c (testDomainSave, testDomainCoreDump): Likewise.
* src/proxy_internal.c (virProxyWriteClientSocket): Likewise.
* src/virsh.c (vshOutputLogFile): Likewise.
* src/console.c (vshRunConsole): Likewise.
* src/util.c (safewrite): Move function definition to ...
* src/util.h (safewrite): ..here, and make it static inline.
* proxy/Makefile.am: Split a long line, and add a newline at EOF.
Signed-off-by: Jim Meyering [EMAIL PROTECTED]
---
.x-sc_avoid_write |2 ++
Makefile.maint| 11 +++
proxy/Makefile.am |5 +++--
proxy/libvirt_proxy.c | 16 +---
qemud/qemud.c | 10 --
src/conf.c|2 +-
src/console.c |6 --
src/proxy_internal.c | 14 --
src/qemu_conf.c |4 ++--
src/qemu_driver.c | 19 +++
src/test.c|9 +
src/util.c| 22 +-
src/util.h| 21 -
src/virsh.c |2 +-
14 files changed, 70 insertions(+), 73 deletions(-)
create mode 100644 .x-sc_avoid_write
diff --git a/.x-sc_avoid_write b/.x-sc_avoid_write
new file mode 100644
index 000..72a196d
--- /dev/null
+++ b/.x-sc_avoid_write
@@ -0,0 +1,2 @@
+^src/util\.c$
+^src/xend_internal\.c$
diff --git a/Makefile.maint b/Makefile.maint
index 4b54baf..afb 100644
--- a/Makefile.maint
+++ b/Makefile.maint
@@ -46,6 +46,17 @@ sc_avoid_if_before_free:
{ echo '$(ME): found useless if before free above' 12; \
exit 1; } || :
+# Avoid uses of write(2). Either switch to streams (fwrite), or use
+# the safewrite wrapper.
+sc_avoid_write:
+ @if $(CVS_LIST_EXCEPT) | grep '\.c$$' /dev/null; then \
+ grep '\write *(' $$($(CVS_LIST_EXCEPT) | grep '\.c$$')
Re: [Libvir] take 2 [Re: write(2) may write less than the total requested
On Wed, 2008-02-20 at 16:42 +0100, Jim Meyering wrote: If I do as you suggest and move the safewrite definition into util.h, and make it static inline, then both problems go away. Make sure to try and build this with gcc-4.3.0 - remember that we un-inlined xstrtol() because of: internal.h:272: error: inlining failed in call to 'xstrtol_i': call is unlikely and code size would grow Again, the option is to stop using -Winline, but it's probably about time we figured out some proper way of sharing code between libvirt.so and virsh etc. without adding it to the ABI. How about a libvirtutil libtool convenience (i.e. static) library? Cheers, Mark. -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] take 2 [Re: write(2) may write less than the total requested
Hi Jim,
comments inline
Jim Meyering wrote:
diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c
index d96d3db..a22ba6c 100644
--- a/proxy/libvirt_proxy.c
+++ b/proxy/libvirt_proxy.c
@@ -2,7 +2,7 @@
* proxy_svr.c: root suid proxy server for Xen access to APIs with no
* side effects from unauthenticated clients.
*
- * Copyright (C) 2006, 2007 Red Hat, Inc.
+ * Copyright (C) 2006, 2007, 2008 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -26,6 +26,7 @@
#include internal.h
#include proxy_internal.h
+#include util.h
#include xen_internal.h
#include xend_internal.h
#include xs_internal.h
@@ -317,19 +318,12 @@ proxyWriteClientSocket(int nr, virProxyPacketPtr req) {
return(-1);
}
-retry:
-ret = write(pollInfos[nr].fd, (char *) req, req-len);
+ret = safewrite(pollInfos[nr].fd, (char *) req, req-len);
if (ret 0) {
Should this check (ret == req-len) instead? safewrite() will return
an error if write() returns an error, regardless of how many bytes are
written, but it's still possible for it to return less than requested
if write() returns 0 (eof?). The behavior of safewrite could be
adjusted to return errors in that case.
-if (errno == EINTR) {
- if (debug 0)
- fprintf(stderr, write socket %d to client %d interrupted\n,
- pollInfos[nr].fd, nr);
- goto retry;
- }
fprintf(stderr, write %d bytes to socket %d from client %d
failed\n,
req-len, pollInfos[nr].fd, nr);
- proxyCloseClientSocket(nr);
- return(-1);
+proxyCloseClientSocket(nr);
+return(-1);
}
if (ret == 0) {
if (debug)
diff --git a/qemud/qemud.c b/qemud/qemud.c
index 3a5e44c..269e9fe 100644
--- a/qemud/qemud.c
+++ b/qemud/qemud.c
@@ -118,7 +118,7 @@ static void sig_handler(int sig) {
return;
origerrno = errno;
-r = write(sigwrite, sigc, 1);
+r = safewrite(sigwrite, sigc, 1);
write() shouldn't do a partial write for size 1, but this is
necessary anyway to help with the EINTR case. Might want to add that
benefit to the log message, it's not just short-writes this protects against.
if (r == -1) {
if (r != 1)?
sig_errors++;
sig_lasterrno = errno;
@@ -1360,11 +1360,9 @@ static int qemudClientWriteBuf(struct qemud_server
*server,
const char *data, int len) {
int ret;
if (!client-tlssession) {
-if ((ret = write(client-fd, data, len)) == -1) {
-if (errno != EAGAIN) {
-qemudLog (QEMUD_ERR, _(write: %s), strerror (errno));
-qemudDispatchClientFailure(server, client);
-}
+if ((ret = safewrite(client-fd, data, len)) == -1) {
+qemudLog (QEMUD_ERR, _(write: %s), strerror (errno));
+qemudDispatchClientFailure(server, client);
ret != len?
return -1;
}
} else {
diff --git a/src/conf.c b/src/conf.c
index e0ecdea..53ea993 100644
--- a/src/conf.c
+++ b/src/conf.c
@@ -904,7 +904,7 @@ __virConfWriteFile(const char *filename, virConfPtr conf)
goto error;
}
-ret = write(fd, buf-content, buf-use);
+ret = safewrite(fd, buf-content, buf-use);
close(fd);
if (ret != (int) buf-use) {
virConfError(NULL, VIR_ERR_WRITE_FAILED, _(failed to save
content), 0);
diff --git a/src/console.c b/src/console.c
index 02a9c7f..1c6cba0 100644
--- a/src/console.c
+++ b/src/console.c
@@ -1,7 +1,7 @@
/*
* console.c: A dumb serial console client
*
- * Copyright (C) 2007 Red Hat, Inc.
+ * Copyright (C) 2007, 2008 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -37,6 +37,7 @@
#include console.h
#include internal.h
+#include util.h
/* ie Ctrl-] as per telnet */
#define CTRL_CLOSE_BRACKET '\35'
@@ -161,7 +162,8 @@ int vshRunConsole(const char *tty) {
while (sent got) {
int done;
-if ((done = write(destfd, buf + sent, got - sent)) = 0)
{
+if ((done = safewrite(destfd, buf + sent, got - sent))
+= 0) {
!= (got - sent)?
fprintf(stderr, _(failure writing output: %s\n),
strerror(errno));
goto cleanup;
diff --git a/src/proxy_internal.c b/src/proxy_internal.c
index bc94763..c3e50c6 100644
--- a/src/proxy_internal.c
+++ b/src/proxy_internal.c
@@ -1,7 +1,7 @@
/*
* proxy_client.c: client side of the communication with the libvirt proxy.
*
- * Copyright (C) 2006 Red Hat, Inc.
+ * Copyright (C) 2006, 2008 Red Hat, Inc.
*
* See COPYING.LIB for the License of this software
*
@@ -26,6 +26,7 @@
#include internal.h
Re: [Libvir] take 2 [Re: write(2) may write less than the total requested
Mark McLoughlin [EMAIL PROTECTED] wrote: On Wed, 2008-02-20 at 16:42 +0100, Jim Meyering wrote: If I do as you suggest and move the safewrite definition into util.h, and make it static inline, then both problems go away. Make sure to try and build this with gcc-4.3.0 - remember that we un-inlined xstrtol() because of: internal.h:272: error: inlining failed in call to 'xstrtol_i': call is unlikely and code size would grow Again, the option is to stop using -Winline, but it's probably about time we figured out some proper way of sharing code between libvirt.so and virsh etc. without adding it to the ABI. How about a libvirtutil libtool convenience (i.e. static) library? This does sound like a better idea. I'll prepare a patch to does that, and probably move at least these into it, too: __virStrToLong_i; __virStrToLong_ull; -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] take 2 [Re: write(2) may write less than the total requested
Jim Paris [EMAIL PROTECTED] wrote:
Jim Meyering wrote:
...
Hi Jim,
-retry:
-ret = write(pollInfos[nr].fd, (char *) req, req-len);
+ret = safewrite(pollInfos[nr].fd, (char *) req, req-len);
if (ret 0) {
Should this check (ret == req-len) instead? safewrite() will return
an error if write() returns an error, regardless of how many bytes are
written,
It *could* perform that test, but I think it is slightly more
maintainable (no duplication of that potentially nontrivial expression)
and just as correct to check only ret 0.
That's why I made changes like this, too:
-if (write(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) !=
sizeof(TEST_SAVE_MAGIC)) {
+if (safewrite(fd, TEST_SAVE_MAGIC, sizeof(TEST_SAVE_MAGIC)) 0) {
Not only that, but the duplication removal makes it more readable because
the reader no longer has to visually ensure that the 3rd arg and the RHS
of the != comparison are the same. As a bonus, that particular change
brings the line length below the 80-col threshold.
but it's still possible for it to return less than requested
if write() returns 0 (eof?).
Really? How? EOF is relevant to read, but not to write(2).
As I see it, calling safewrite can have only two outcomes:
- return -1 to indicate failure
- return the requested byte count (arg #3, count, which is non-negative)
The only way safewrite can return 0 is if its count argument is also 0,
and that's not a failure. This is because write itself can return 0 only
if its count is also 0.
--
Libvir-list mailing list
Libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [Libvir] take 2 [Re: write(2) may write less than the total requested
Jim Meyering wrote: It *could* perform that test, but I think it is slightly more maintainable (no duplication of that potentially nontrivial expression) and just as correct to check only ret 0. Not having the duplicated expression is certainly good, if it's correct to do so (and it seems you're right). but it's still possible for it to return less than requested if write() returns 0 (eof?). Really? How? EOF is relevant to read, but not to write(2). As I see it, calling safewrite can have only two outcomes: - return -1 to indicate failure - return the requested byte count (arg #3, count, which is non-negative) The only way safewrite can return 0 is if its count argument is also 0, and that's not a failure. This is because write itself can return 0 only if its count is also 0. Hmm, I was thinking write might return 0 for closed pipes and similar but you're right, that's different from EOF and should return error. http://www.opengroup.org/onlinepubs/95399/functions/write.html says: Where this volume of IEEE Std 1003.1-2001 requires -1 to be returned and errno set to [EAGAIN], most historical implementations return zero (with the O_NDELAY flag set, which is the historical predecessor of O_NONBLOCK, but is not itself in this volume of IEEE Std 1003.1-2001). The error indications in this volume of IEEE Std 1003.1-2001 were chosen so that an application can distinguish these cases from end-of-file. so we're safe here. It does bring up the point that safewrite() doesn't handle EAGAIN and might not be appropriate for non-blocking fds. The sigwrite pipe is non-blocking. At quick glance, the qemud fds might be that way too? It also makes me notice that we have 3 *SetNonBlock functions, two with the same name even... -jim -- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
