Based on a warning from coverity. The safe* functions guarantee complete transactions on success, but don't guarantee freedom from failure. * src/util/util.h (saferead, safewrite, safezero): Add ATTRIBUTE_RETURN_CHECK. * src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Ignore some failures. (remoteIOReadBuffer): Adjust error messages on read failure. * daemon/event.c (virEventHandleWakeup): Ignore read failure. --- daemon/event.c | 5 +++-- src/remote/remote_driver.c | 22 ++++++++++++++++------ src/util/util.h | 9 ++++++--- 3 files changed, 25 insertions(+), 11 deletions(-) diff --git a/daemon/event.c b/daemon/event.c index 2218a3e..6971409 100644 --- a/daemon/event.c +++ b/daemon/event.c @@ -1,8 +1,8 @@ /* * event.c: event loop for monitoring file handles * + * Copyright (C) 2007, 2010 Red Hat, Inc. * Copyright (C) 2007 Daniel P. Berrange - * Copyright (C) 2007 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 @@ -35,6 +35,7 @@ #include "event.h" #include "memory.h" #include "util.h" +#include "ignore-value.h" #define EVENT_DEBUG(fmt, ...) DEBUG(fmt, __VA_ARGS__) @@ -630,7 +631,7 @@ static void virEventHandleWakeup(int watch ATTRIBUTE_UNUSED, { char c; virEventLock(); - saferead(fd, &c, sizeof(c)); + ignore_value(saferead(fd, &c, sizeof(c))); virEventUnlock(); } diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index f514e1d..ebcfcd8 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8877,7 +8877,11 @@ remoteIOReadBuffer(struct private_data *priv, char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + virReportSystemError(errno, "%s", + _("cannot recv data")); + return -1; + } } virReportSystemError(errno, @@ -8886,7 +8890,12 @@ remoteIOReadBuffer(struct private_data *priv, } else { char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + remoteError(VIR_ERR_SYSTEM_ERROR, + _("server closed connection: %s"), + virStrerror(errno, errout, sizeof errout)); + return -1; + } } remoteError(VIR_ERR_SYSTEM_ERROR, @@ -9499,7 +9508,7 @@ remoteIOEventLoop(virConnectPtr conn, sigaddset (&blockedsigs, SIGWINCH); sigaddset (&blockedsigs, SIGCHLD); sigaddset (&blockedsigs, SIGPIPE); - ignore_value (pthread_sigmask(SIG_BLOCK, &blockedsigs, &oldmask)); + ignore_value(pthread_sigmask(SIG_BLOCK, &blockedsigs, &oldmask)); #endif repoll: @@ -9508,14 +9517,15 @@ remoteIOEventLoop(virConnectPtr conn, goto repoll; #ifdef HAVE_PTHREAD_H - ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); + ignore_value(pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); #endif remoteDriverLock(priv); if (fds[1].revents) { DEBUG0("Woken up from poll by other thread"); - saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)); + ignore_value(saferead(priv->wakeupReadFD, &ignore, + sizeof(ignore))); } if (ret < 0) { @@ -9659,7 +9669,7 @@ remoteIO(virConnectPtr conn, priv->waitDispatch = thiscall; /* Force other thread to wakup from poll */ - safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + ignore_value(safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */ diff --git a/src/util/util.h b/src/util/util.h index c256117..4f0b233 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -2,6 +2,7 @@ /* * utils.h: common, generic utility functions * + * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * @@ -31,9 +32,11 @@ # include <sys/select.h> # include <sys/types.h> -int saferead(int fd, void *buf, size_t count); -ssize_t safewrite(int fd, const void *buf, size_t count); -int safezero(int fd, int flags, off_t offset, off_t len); +int saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; +ssize_t safewrite(int fd, const void *buf, size_t count) + ATTRIBUTE_RETURN_CHECK; +int safezero(int fd, int flags, off_t offset, off_t len) + ATTRIBUTE_RETURN_CHECK; enum { VIR_EXEC_NONE = 0, -- 1.6.6.1

Eric Blake wrote: This looks fine, but "errno ? errno : 0" is equivalent to just "errno". Which makes me think you'll want to separate the saferead-fails case (where errno is nonzero) from the saferead-returns-non-negative-<=-sizeof-ignore case (in which case virReportSystemError would print "Success" for errno=0).

Eric Blake wrote: Thanks. That looks better. No more "Success" diagnostic upon error.