[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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"
>  #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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]