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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
-jim
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list