[Libvir] [PATCH] write(2) may write less than the total requested
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. * qemud/qemud.c (sig_handler): * src/conf.c (__virConfWriteFile): * src/proxy_internal.c (virProxyWriteClientSocket): * src/qemu_conf.c (qemudSaveConfig, qemudSaveNetworkConfig): * src/qemu_driver.c (qemudWaitForMonitor, qemudStartVMDaemon) (qemudVMData, PROC_IP_FORWARD): * src/test.c (testDomainSave, testDomainCoreDump): * proxy/libvirt_proxy.c (proxyWriteClientSocket): * proxy/Makefile.am (libvirt_proxy_SOURCES): Add src/util.c. * Makefile.maint (sc_avoid_write): New rule. * .x-sc_avoid_write: New file. Record the two legitimate exemptions, and two that are temporarily grandfathered in. Signed-off-by: Jim Meyering <meyering@redhat.com> --- .x-sc_avoid_write | 4 ++++ Makefile.maint | 11 +++++++++++ proxy/Makefile.am | 5 +++-- proxy/libvirt_proxy.c | 15 ++++----------- qemud/qemud.c | 10 ++++------ src/conf.c | 2 +- src/proxy_internal.c | 13 +++---------- src/qemu_conf.c | 4 ++-- src/qemu_driver.c | 19 +++++++------------ src/test.c | 8 ++++---- 10 files changed, 43 insertions(+), 48 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 0000000..0933e20 --- /dev/null +++ b/.x-sc_avoid_write @@ -0,0 +1,4 @@ +^src/util\.c$ +^src/xend_internal\.c$ +^src/virsh\.c$ +^src/console\.c$ diff --git a/Makefile.maint b/Makefile.maint index 4b54baf..afddddb 100644 --- a/Makefile.maint +++ b/Makefile.maint @@ -46,6 +46,17 @@ sc_avoid_if_before_free: { echo '$(ME): found useless "if" before "free" above' 1>&2; \ 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$$') && \ + { echo "$(ME): the above files use write;" \ + " consider using the safewrite wrapper instead" \ + 1>&2; exit 1; } || :; \ + else :; \ + fi + sc_cast_of_argument_to_free: @grep -nE '\<free \(\(' $$($(CVS_LIST_EXCEPT)) && \ { echo '$(ME): don'\''t cast free argument' 1>&2; \ diff --git a/proxy/Makefile.am b/proxy/Makefile.am index ff9a3eb..6827f36 100644 --- a/proxy/Makefile.am +++ b/proxy/Makefile.am @@ -12,11 +12,12 @@ libexec_PROGRAMS = libvirt_proxy libvirt_proxy_SOURCES = libvirt_proxy.c @top_srcdir@/src/xend_internal.c \ @top_srcdir@/src/xen_internal.c @top_srcdir@/src/virterror.c \ @top_srcdir@/src/sexpr.c @top_srcdir@/src/xml.c \ - @top_srcdir@/src/xs_internal.c @top_srcdir@/src/buf.c @top_srcdir@/src/uuid.c + @top_srcdir@/src/xs_internal.c @top_srcdir@/src/buf.c \ + @top_srcdir@/src/util.c @top_srcdir@/src/uuid.c libvirt_proxy_LDFLAGS = $(WARN_CFLAGS) libvirt_proxy_DEPENDENCIES = libvirt_proxy_LDADD = install-exec-hook: chmod u+s $(DESTDIR)$(libexecdir)/libvirt_proxy -endif \ No newline at end of file +endif diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c index d96d3db..12d0f35 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 * @@ -317,19 +317,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) { - 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); 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); 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/proxy_internal.c b/src/proxy_internal.c index bc94763..5a14e54 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 * @@ -345,17 +345,10 @@ virProxyWriteClientSocket(int fd, const char *data, int len) { if ((fd < 0) || (data == NULL) || (len < 0)) return(-1); -retry: - ret = write(fd, data, len); + ret = safewrite(fd, data, len); if (ret < 0) { - if (errno == EINTR) { - if (debug > 0) - fprintf(stderr, "write socket %d, %d bytes interrupted\n", - fd, len); - goto retry; - } fprintf(stderr, _("Failed to write to socket %d\n"), fd); - return(-1); + return(-1); } if (debug) fprintf(stderr, "wrote %d bytes to socket %d\n", diff --git a/src/qemu_conf.c b/src/qemu_conf.c index e39c7bc..ff6a92e 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1866,7 +1866,7 @@ static int qemudSaveConfig(virConnectPtr conn, } towrite = strlen(xml); - if (write(fd, xml, towrite) != towrite) { + if (safewrite(fd, xml, towrite) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write config file %s: %s", vm->configFile, strerror(errno)); @@ -2089,7 +2089,7 @@ static int qemudSaveNetworkConfig(virConnectPtr conn, } towrite = strlen(xml); - if (write(fd, xml, towrite) != towrite) { + if (safewrite(fd, xml, towrite) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write config file %s: %s", network->configFile, strerror(errno)); diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 15cd52c..85d042f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -539,11 +539,9 @@ static int qemudWaitForMonitor(virConnectPtr conn, "console"); buf[sizeof(buf)-1] = '\0'; - retry: - if (write(vm->logfile, buf, strlen(buf)) < 0) { + + if (safewrite(vm->logfile, buf, strlen(buf)) < 0) { /* Log, but ignore failures to write logfile for VM */ - if (errno == EINTR) - goto retry; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s"), strerror(errno)); } @@ -656,15 +654,15 @@ static int qemudStartVMDaemon(virConnectPtr conn, tmp = argv; while (*tmp) { - if (write(vm->logfile, *tmp, strlen(*tmp)) < 0) + if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); - if (write(vm->logfile, " ", 1) < 0) + if (safewrite(vm->logfile, " ", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); tmp++; } - if (write(vm->logfile, "\n", 1) < 0) + if (safewrite(vm->logfile, "\n", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); @@ -733,11 +731,8 @@ static int qemudVMData(struct qemud_driver *driver ATTRIBUTE_UNUSED, } buf[ret] = '\0'; - retry: - if (write(vm->logfile, buf, ret) < 0) { + if (safewrite(vm->logfile, buf, ret) < 0) { /* Log, but ignore failures to write logfile for VM */ - if (errno == EINTR) - goto retry; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s"), strerror(errno)); } @@ -1113,7 +1108,7 @@ qemudEnableIpForwarding(void) if ((fd = open(PROC_IP_FORWARD, O_WRONLY|O_TRUNC)) == -1) return 0; - if (write(fd, "1\n", 2) < 0) + if (safewrite(fd, "1\n", 2) < 0) ret = 0; close (fd); diff --git a/src/test.c b/src/test.c index 003d6b7..3dd1007 100644 --- a/src/test.c +++ b/src/test.c @@ -1293,19 +1293,19 @@ static int testDomainSave(virDomainPtr domain, return (-1); } len = strlen(xml); - 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) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write header"); close(fd); return (-1); } - if (write(fd, (char*)&len, sizeof(len)) != sizeof(len)) { + if (safewrite(fd, (char*)&len, sizeof(len)) < 0) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write metadata length"); close(fd); return (-1); } - if (write(fd, xml, len) != len) { + if (safewrite(fd, xml, len) < 0) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write metadata"); free(xml); @@ -1398,7 +1398,7 @@ static int testDomainCoreDump(virDomainPtr domain, "cannot save domain core"); return (-1); } - 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) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write header"); close(fd); -- 1.5.4.2.134.g82883
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 Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard <veillard@redhat.com> 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 <meyering@redhat.com> --- .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 0000000..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..afddddb 100644 --- a/Makefile.maint +++ b/Makefile.maint @@ -46,6 +46,17 @@ sc_avoid_if_before_free: { echo '$(ME): found useless "if" before "free" above' 1>&2; \ 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$$') && \ + { echo "$(ME): the above files use write;" \ + " consider using the safewrite wrapper instead" \ + 1>&2; exit 1; } || :; \ + else :; \ + fi + sc_cast_of_argument_to_free: @grep -nE '\<free \(\(' $$($(CVS_LIST_EXCEPT)) && \ { echo '$(ME): don'\''t cast free argument' 1>&2; \ diff --git a/proxy/Makefile.am b/proxy/Makefile.am index ff9a3eb..006e503 100644 --- a/proxy/Makefile.am +++ b/proxy/Makefile.am @@ -12,11 +12,12 @@ libexec_PROGRAMS = libvirt_proxy libvirt_proxy_SOURCES = libvirt_proxy.c @top_srcdir@/src/xend_internal.c \ @top_srcdir@/src/xen_internal.c @top_srcdir@/src/virterror.c \ @top_srcdir@/src/sexpr.c @top_srcdir@/src/xml.c \ - @top_srcdir@/src/xs_internal.c @top_srcdir@/src/buf.c @top_srcdir@/src/uuid.c + @top_srcdir@/src/xs_internal.c @top_srcdir@/src/buf.c \ + @top_srcdir@/src/uuid.c libvirt_proxy_LDFLAGS = $(WARN_CFLAGS) libvirt_proxy_DEPENDENCIES = libvirt_proxy_LDADD = install-exec-hook: chmod u+s $(DESTDIR)$(libexecdir)/libvirt_proxy -endif \ No newline at end of file +endif 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) { - 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); 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); 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) { 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" #include "driver.h" #include "proxy_internal.h" +#include "util.h" #include "xen_unified.h" #define STANDALONE @@ -345,17 +346,10 @@ virProxyWriteClientSocket(int fd, const char *data, int len) { if ((fd < 0) || (data == NULL) || (len < 0)) return(-1); -retry: - ret = write(fd, data, len); + ret = safewrite(fd, data, len); if (ret < 0) { - if (errno == EINTR) { - if (debug > 0) - fprintf(stderr, "write socket %d, %d bytes interrupted\n", - fd, len); - goto retry; - } fprintf(stderr, _("Failed to write to socket %d\n"), fd); - return(-1); + return(-1); } if (debug) fprintf(stderr, "wrote %d bytes to socket %d\n", diff --git a/src/qemu_conf.c b/src/qemu_conf.c index e39c7bc..ff6a92e 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1866,7 +1866,7 @@ static int qemudSaveConfig(virConnectPtr conn, } towrite = strlen(xml); - if (write(fd, xml, towrite) != towrite) { + if (safewrite(fd, xml, towrite) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write config file %s: %s", vm->configFile, strerror(errno)); @@ -2089,7 +2089,7 @@ static int qemudSaveNetworkConfig(virConnectPtr conn, } towrite = strlen(xml); - if (write(fd, xml, towrite) != towrite) { + if (safewrite(fd, xml, towrite) < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write config file %s: %s", network->configFile, strerror(errno)); diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 15cd52c..85d042f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -539,11 +539,9 @@ static int qemudWaitForMonitor(virConnectPtr conn, "console"); buf[sizeof(buf)-1] = '\0'; - retry: - if (write(vm->logfile, buf, strlen(buf)) < 0) { + + if (safewrite(vm->logfile, buf, strlen(buf)) < 0) { /* Log, but ignore failures to write logfile for VM */ - if (errno == EINTR) - goto retry; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s"), strerror(errno)); } @@ -656,15 +654,15 @@ static int qemudStartVMDaemon(virConnectPtr conn, tmp = argv; while (*tmp) { - if (write(vm->logfile, *tmp, strlen(*tmp)) < 0) + if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); - if (write(vm->logfile, " ", 1) < 0) + if (safewrite(vm->logfile, " ", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); tmp++; } - if (write(vm->logfile, "\n", 1) < 0) + if (safewrite(vm->logfile, "\n", 1) < 0) qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); @@ -733,11 +731,8 @@ static int qemudVMData(struct qemud_driver *driver ATTRIBUTE_UNUSED, } buf[ret] = '\0'; - retry: - if (write(vm->logfile, buf, ret) < 0) { + if (safewrite(vm->logfile, buf, ret) < 0) { /* Log, but ignore failures to write logfile for VM */ - if (errno == EINTR) - goto retry; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s"), strerror(errno)); } @@ -1113,7 +1108,7 @@ qemudEnableIpForwarding(void) if ((fd = open(PROC_IP_FORWARD, O_WRONLY|O_TRUNC)) == -1) return 0; - if (write(fd, "1\n", 2) < 0) + if (safewrite(fd, "1\n", 2) < 0) ret = 0; close (fd); diff --git a/src/test.c b/src/test.c index 003d6b7..346774b 100644 --- a/src/test.c +++ b/src/test.c @@ -42,6 +42,7 @@ #include "test.h" #include "xml.h" #include "buf.h" +#include "util.h" #include "uuid.h" /* Flags that determine the action to take on a shutdown or crash of a domain @@ -1293,19 +1294,19 @@ static int testDomainSave(virDomainPtr domain, return (-1); } len = strlen(xml); - 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) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write header"); close(fd); return (-1); } - if (write(fd, (char*)&len, sizeof(len)) != sizeof(len)) { + if (safewrite(fd, (char*)&len, sizeof(len)) < 0) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write metadata length"); close(fd); return (-1); } - if (write(fd, xml, len) != len) { + if (safewrite(fd, xml, len) < 0) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write metadata"); free(xml); @@ -1398,7 +1399,7 @@ static int testDomainCoreDump(virDomainPtr domain, "cannot save domain core"); return (-1); } - 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) { testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write header"); close(fd); diff --git a/src/util.c b/src/util.c index f082984..c813a4c 100644 --- a/src/util.c +++ b/src/util.c @@ -1,7 +1,7 @@ /* * utils.c: common, generic utility functions * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain @@ -295,26 +295,6 @@ int saferead(int fd, void *buf, size_t count) return nread; } -/* Like write(), but restarts after EINTR */ -ssize_t safewrite(int fd, const void *buf, size_t count) -{ - size_t nwritten = 0; - while (count > 0) { - int r = write(fd, buf, count); - - if (r < 0 && errno == EINTR) - continue; - if (r < 0) - return r; - if (r == 0) - return nwritten; - buf = (unsigned char *)buf + r; - count -= r; - nwritten += r; - } - return nwritten; -} - int __virFileReadAll(const char *path, int maxlen, diff --git a/src/util.h b/src/util.h index da1f5b0..218ab1b 100644 --- a/src/util.h +++ b/src/util.h @@ -31,7 +31,26 @@ int virExecNonBlock(virConnectPtr conn, char **argv, int *retpid, int infd, int int virRun(virConnectPtr conn, char **argv, int *status); int saferead(int fd, void *buf, size_t count); -ssize_t safewrite(int fd, const void *buf, size_t count); + +/* Like write(), but restarts after EINTR */ +static inline ssize_t safewrite(int fd, const void *buf, size_t count) +{ + size_t nwritten = 0; + while (count > 0) { + int r = write(fd, buf, count); + + if (r < 0 && errno == EINTR) + continue; + if (r < 0) + return r; + if (r == 0) + return nwritten; + buf = (unsigned char *)buf + r; + count -= r; + nwritten += r; + } + return nwritten; +} int __virFileReadAll(const char *path, int maxlen, diff --git a/src/virsh.c b/src/virsh.c index 730f251..b67e63c 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -4582,7 +4582,7 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, va_list snprintf(msg_buf + strlen(msg_buf), sizeof(msg_buf) - strlen(msg_buf), "\n"); /* write log */ - if (write(ctl->log_fd, msg_buf, strlen(msg_buf)) == -1) { + if (safewrite(ctl->log_fd, msg_buf, strlen(msg_buf)) < 0) { vshCloseLogFile(ctl); vshError(ctl, FALSE, "%s", _("failed to write the log file")); } -- 1.5.4.2.134.g82883
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.
Mark McLoughlin <markmc@redhat.com> 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;
Jim Meyering <jim@meyering.net> wrote:
Mark McLoughlin <markmc@redhat.com> 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;
Actually, it's not so simple. Using a libtool convenience library would work, but to be useful, it would have to expose the safewrite function to all users of libvirt.la. And, since we seem to want to keep -Winline, we probably don't want "static inline" functions, so here's what I've done: #1 Allow safewrite to be used both by library and non-library code: Subject: [PATCH] Move safewrite and saferead to a separate file. #2 Fix the uses of write that started all this: Subject: [PATCH] Use safewrite in place of write, in many cases. Details in the following patches.
On Wed, Feb 20, 2008 at 04:42:12PM +0100, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
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.
That's not what I suggested. Just make a copy of the function in virsh.c Inlining IMHO raises more problems than it solves. In that case duplicating the code to clean up virsh.c own code does not sound unreasonable. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
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" #include "driver.h" #include "proxy_internal.h" +#include "util.h" #include "xen_unified.h"
#define STANDALONE @@ -345,17 +346,10 @@ virProxyWriteClientSocket(int fd, const char *data, int len) { if ((fd < 0) || (data == NULL) || (len < 0)) return(-1);
-retry: - ret = write(fd, data, len); + ret = safewrite(fd, data, len); if (ret < 0) {
ret != len? Or at least (ret <= 0).
- if (errno == EINTR) { - if (debug > 0) - fprintf(stderr, "write socket %d, %d bytes interrupted\n", - fd, len); - goto retry; - } fprintf(stderr, _("Failed to write to socket %d\n"), fd); - return(-1); + return(-1); } if (debug) fprintf(stderr, "wrote %d bytes to socket %d\n", diff --git a/src/qemu_conf.c b/src/qemu_conf.c index e39c7bc..ff6a92e 100644 --- a/src/qemu_conf.c +++ b/src/qemu_conf.c @@ -1866,7 +1866,7 @@ static int qemudSaveConfig(virConnectPtr conn, }
towrite = strlen(xml); - if (write(fd, xml, towrite) != towrite) { + if (safewrite(fd, xml, towrite) < 0) {
Stick with "!= towrite" or at least "<= 0"
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write config file %s: %s", vm->configFile, strerror(errno)); @@ -2089,7 +2089,7 @@ static int qemudSaveNetworkConfig(virConnectPtr conn, }
towrite = strlen(xml); - if (write(fd, xml, towrite) != towrite) { + if (safewrite(fd, xml, towrite) < 0) {
ditto
qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write config file %s: %s", network->configFile, strerror(errno)); diff --git a/src/qemu_driver.c b/src/qemu_driver.c index 15cd52c..85d042f 100644 --- a/src/qemu_driver.c +++ b/src/qemu_driver.c @@ -539,11 +539,9 @@ static int qemudWaitForMonitor(virConnectPtr conn, "console");
buf[sizeof(buf)-1] = '\0'; - retry: - if (write(vm->logfile, buf, strlen(buf)) < 0) { + + if (safewrite(vm->logfile, buf, strlen(buf)) < 0) {
!= strlen(buf)
/* Log, but ignore failures to write logfile for VM */ - if (errno == EINTR) - goto retry; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s"), strerror(errno)); } @@ -656,15 +654,15 @@ static int qemudStartVMDaemon(virConnectPtr conn,
tmp = argv; while (*tmp) { - if (write(vm->logfile, *tmp, strlen(*tmp)) < 0) + if (safewrite(vm->logfile, *tmp, strlen(*tmp)) < 0)
!= strlen(*tmp)
qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); - if (write(vm->logfile, " ", 1) < 0) + if (safewrite(vm->logfile, " ", 1) < 0)
!= 1
qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno)); tmp++; } - if (write(vm->logfile, "\n", 1) < 0) + if (safewrite(vm->logfile, "\n", 1) < 0)
!= 1
qemudLog(QEMUD_WARN, _("Unable to write argv to logfile %d: %s"), errno, strerror(errno));
@@ -733,11 +731,8 @@ static int qemudVMData(struct qemud_driver *driver ATTRIBUTE_UNUSED, } buf[ret] = '\0';
- retry: - if (write(vm->logfile, buf, ret) < 0) { + if (safewrite(vm->logfile, buf, ret) < 0) {
!= ret
/* Log, but ignore failures to write logfile for VM */ - if (errno == EINTR) - goto retry; qemudLog(QEMUD_WARN, _("Unable to log VM console data: %s"), strerror(errno)); } @@ -1113,7 +1108,7 @@ qemudEnableIpForwarding(void) if ((fd = open(PROC_IP_FORWARD, O_WRONLY|O_TRUNC)) == -1) return 0;
- if (write(fd, "1\n", 2) < 0) + if (safewrite(fd, "1\n", 2) < 0)
!= 2
ret = 0;
close (fd); diff --git a/src/test.c b/src/test.c index 003d6b7..346774b 100644 --- a/src/test.c +++ b/src/test.c @@ -42,6 +42,7 @@ #include "test.h" #include "xml.h" #include "buf.h" +#include "util.h" #include "uuid.h"
/* Flags that determine the action to take on a shutdown or crash of a domain @@ -1293,19 +1294,19 @@ static int testDomainSave(virDomainPtr domain, return (-1); } len = strlen(xml); - 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) {
keep the != sizeof(TEST_SAVE_MAGIC)
testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write header"); close(fd); return (-1); } - if (write(fd, (char*)&len, sizeof(len)) != sizeof(len)) { + if (safewrite(fd, (char*)&len, sizeof(len)) < 0) {
keep the != sizeof(len)
testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write metadata length"); close(fd); return (-1); } - if (write(fd, xml, len) != len) { + if (safewrite(fd, xml, len) < 0) {
keep the != len
testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write metadata"); free(xml); @@ -1398,7 +1399,7 @@ static int testDomainCoreDump(virDomainPtr domain, "cannot save domain core"); return (-1); } - 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) {
keep the != sizeof(TEST_SAVE_MAGIC)
testError(domain->conn, domain, NULL, VIR_ERR_INTERNAL_ERROR, "cannot write header"); close(fd); diff --git a/src/util.c b/src/util.c index f082984..c813a4c 100644 --- a/src/util.c +++ b/src/util.c @@ -1,7 +1,7 @@ /* * utils.c: common, generic utility functions * - * Copyright (C) 2006, 2007 Red Hat, Inc. + * Copyright (C) 2006, 2007, 2008 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain @@ -295,26 +295,6 @@ int saferead(int fd, void *buf, size_t count) return nread; }
-/* Like write(), but restarts after EINTR */ -ssize_t safewrite(int fd, const void *buf, size_t count) -{ - size_t nwritten = 0; - while (count > 0) { - int r = write(fd, buf, count); - - if (r < 0 && errno == EINTR) - continue; - if (r < 0) - return r; - if (r == 0) - return nwritten; - buf = (unsigned char *)buf + r; - count -= r; - nwritten += r; - } - return nwritten; -} -
int __virFileReadAll(const char *path, int maxlen, diff --git a/src/util.h b/src/util.h index da1f5b0..218ab1b 100644 --- a/src/util.h +++ b/src/util.h @@ -31,7 +31,26 @@ int virExecNonBlock(virConnectPtr conn, char **argv, int *retpid, int infd, int int virRun(virConnectPtr conn, char **argv, int *status);
int saferead(int fd, void *buf, size_t count); -ssize_t safewrite(int fd, const void *buf, size_t count); + +/* Like write(), but restarts after EINTR */ +static inline ssize_t safewrite(int fd, const void *buf, size_t count) +{ + size_t nwritten = 0; + while (count > 0) { + int r = write(fd, buf, count); + + if (r < 0 && errno == EINTR) + continue; + if (r < 0) + return r; + if (r == 0) + return nwritten; + buf = (unsigned char *)buf + r; + count -= r; + nwritten += r; + } + return nwritten; +}
int __virFileReadAll(const char *path, int maxlen, diff --git a/src/virsh.c b/src/virsh.c index 730f251..b67e63c 100644 --- a/src/virsh.c +++ b/src/virsh.c @@ -4582,7 +4582,7 @@ vshOutputLogFile(vshControl *ctl, int log_level, const char *msg_format, va_list snprintf(msg_buf + strlen(msg_buf), sizeof(msg_buf) - strlen(msg_buf), "\n");
/* write log */ - if (write(ctl->log_fd, msg_buf, strlen(msg_buf)) == -1) { + if (safewrite(ctl->log_fd, msg_buf, strlen(msg_buf)) < 0) {
!= strlen(msg_buf)
vshCloseLogFile(ctl); vshError(ctl, FALSE, "%s", _("failed to write the log file")); } -- 1.5.4.2.134.g82883
-- Libvir-list mailing list Libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-jim
Jim Paris <jim@jtan.com> 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.
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/000095399/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
participants (4)
-
Daniel Veillard -
Jim Meyering -
Jim Paris -
Mark McLoughlin