[libvirt] [PATCHv2] util: ensure safe{read, write, zero} return is checked
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. --- Changes from previous attempt: rework remote_driver to no longer trigger 'make syntax-check' failures. This can wait until after 0.7.7. daemon/event.c | 5 +++-- src/remote/remote_driver.c | 18 ++++++++++++++---- src/util/util.h | 9 ++++++--- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/daemon/event.c b/daemon/event.c index 2218a3e..36e9dd3 100644 --- a/daemon/event.c +++ b/daemon/event.c @@ -1,13 +1,13 @@ /* * 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 * License as published by the Free Software Foundation; either * version 2.1 of the License, or (at your option) any later version. * * This library is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of @@ -30,16 +30,17 @@ #include <errno.h> #include <unistd.h> #include "threads.h" #include "logging.h" #include "event.h" #include "memory.h" #include "util.h" +#include "ignore-value.h" #define EVENT_DEBUG(fmt, ...) DEBUG(fmt, __VA_ARGS__) static int virEventInterruptLocked(void); /* State for a single file handle being monitored */ struct virEventHandle { int watch; @@ -625,17 +626,17 @@ error_unlocked: static void virEventHandleWakeup(int watch ATTRIBUTE_UNUSED, int fd, int events ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { char c; virEventLock(); - saferead(fd, &c, sizeof(c)); + ignore_value (saferead(fd, &c, sizeof(c))); virEventUnlock(); } int virEventInit(void) { if (pthread_mutex_init(&eventLoop.lock, NULL) != 0) return -1; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b7b2e09..d9cae8b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7868,26 +7868,35 @@ remoteIOReadBuffer(virConnectPtr conn, if (ret == -1) { if (errno == EINTR) goto resend; if (errno == EWOULDBLOCK) return 0; 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: unknown reason")); + return -1; + } } virReportSystemError(errno, _("cannot recv data: %s"), errout); } else { char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + errorf (in_open ? NULL : conn, + VIR_ERR_SYSTEM_ERROR, + _("server closed connection: unknown reason")); + return -1; + } } errorf (in_open ? NULL : conn, VIR_ERR_SYSTEM_ERROR, _("server closed connection: %s"), errout); } return -1; } @@ -8490,17 +8499,18 @@ remoteIOEventLoop(virConnectPtr conn, goto repoll; ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL)); 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) { if (errno == EWOULDBLOCK) continue; virReportSystemError(errno, "%s", _("poll on socket failed")); goto error; @@ -8634,17 +8644,17 @@ remoteIO(virConnectPtr conn, while (tmp && tmp->next) tmp = tmp->next; if (tmp) tmp->next = thiscall; else 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... */ if (virCondWait(&thiscall->cond, &priv->lock) < 0) { if (priv->waitDispatch == thiscall) { priv->waitDispatch = thiscall->next; } else { tmp = priv->waitDispatch; diff --git a/src/util/util.h b/src/util/util.h index cc05abe..34b77fa 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -1,12 +1,13 @@ /* * utils.h: common, generic utility functions * + * Copyright (C) 2010 Red Hat, Inc. * Copyright (C) 2006, 2007 Binary Karma * Copyright (C) 2006 Shuveb Hussain * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public * License as published by the Free Software Foundation; either * version 2.1 of the License, or (at your option) any later version. * @@ -26,19 +27,21 @@ #define __VIR_UTIL_H__ #include "verify.h" #include "internal.h" #include <unistd.h> #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, VIR_EXEC_NONBLOCK = (1 << 0), VIR_EXEC_DAEMON = (1 << 1), VIR_EXEC_CLEAR_CAPS = (1 << 2), }; -- 1.6.6.1
On 03/03/2010 01:47 PM, Eric Blake wrote:
Based on a warning from coverity. The safe* functions guarantee complete transactions on success, but don't guarantee freedom from failure.
<snip>
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b7b2e09..d9cae8b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7868,26 +7868,35 @@ remoteIOReadBuffer(virConnectPtr conn, if (ret == -1) { if (errno == EINTR) goto resend; if (errno == EWOULDBLOCK) return 0;
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: unknown reason")); + return -1; + } }
Minor point, but is "unknown reason" really the right thing we want to say here? Won't we know the reason from errno?
virReportSystemError(errno, _("cannot recv data: %s"), errout);
} else { char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + errorf (in_open ? NULL : conn, + VIR_ERR_SYSTEM_ERROR, + _("server closed connection: unknown reason")); + return -1; + } }
Same here.
errorf (in_open ? NULL : conn, VIR_ERR_SYSTEM_ERROR, _("server closed connection: %s"), errout); } return -1; } @@ -8490,17 +8499,18 @@ remoteIOEventLoop(virConnectPtr conn, goto repoll;
ignore_value (pthread_sigmask(SIG_SETMASK, &oldmask, NULL));
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)));
Hm. Are we sure we want to ignore the return value here? On the one hand, the fact that we failed to read this byte is irrelevant; it has done it's job to wake us out of poll. On the other hand, if we can't read this one byte, that probably means something more serious is wrong and we might (probably will?) have problems later. Should we goto error here?
}
if (ret < 0) { if (errno == EWOULDBLOCK) continue; virReportSystemError(errno, "%s", _("poll on socket failed")); goto error; @@ -8634,17 +8644,17 @@ remoteIO(virConnectPtr conn, while (tmp && tmp->next) tmp = tmp->next; if (tmp) tmp->next = thiscall; else priv->waitDispatch = thiscall;
/* Force other thread to wakup from poll */ - safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + ignore_value (safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)));
Same here. -- Chris Lalancette
On 03/09/2010 11:53 AM, Chris Lalancette wrote:
if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + virReportSystemError(errno, "%s", + _("cannot recv data: unknown reason")); + return -1; + } }
Minor point, but is "unknown reason" really the right thing we want to say here? Won't we know the reason from errno?
Valid point. v3 coming up later today, after I have lunch.
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)));
Hm. Are we sure we want to ignore the return value here? On the one hand, the fact that we failed to read this byte is irrelevant; it has done it's job to wake us out of poll. On the other hand, if we can't read this one byte, that probably means something more serious is wrong and we might (probably will?) have problems later. Should we goto error here?
Not the first instance of this; see, for example, daemon/event.c, virEventHandleWakeup(). If we change how wakeups are handled, it should be a separate patch, applied to all instances. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 03/09/2010 02:09 PM, Eric Blake wrote:
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)));
Hm. Are we sure we want to ignore the return value here? On the one hand, the fact that we failed to read this byte is irrelevant; it has done it's job to wake us out of poll. On the other hand, if we can't read this one byte, that probably means something more serious is wrong and we might (probably will?) have problems later. Should we goto error here?
Not the first instance of this; see, for example, daemon/event.c, virEventHandleWakeup(). If we change how wakeups are handled, it should be a separate patch, applied to all instances.
Yes and no. Yes, virEventHandleWakeup() does have a similar problem. However, the callback function is an exported prototype, so we can't change it to have a return value now (and though we might be able to play tricks with the opaque pointer, we can't guarantee all callback users will follow the tricks). Personally I would fix this one now and we could think about the wakeup problem with virEventHandleWakeup() later, but I won't block the patch for the (pretty minor) point. -- Chris Lalancette
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. --- This is the minimalistic approach for plugging the coverity report. It does nothing about the fact that ignoring system errors might be a bad idea. daemon/event.c | 5 +++-- src/remote/remote_driver.c | 19 +++++++++++++++---- src/util/util.h | 9 ++++++--- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/daemon/event.c b/daemon/event.c index 2218a3e..36e9dd3 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 11513bd..57cf8a2 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7873,7 +7873,11 @@ remoteIOReadBuffer(virConnectPtr conn, 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, @@ -7882,7 +7886,13 @@ remoteIOReadBuffer(virConnectPtr conn, } else { char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + errorf (in_open ? NULL : conn, + VIR_ERR_SYSTEM_ERROR, + _("server closed connection: %s"), + strerror(errno)); + return -1; + } } errorf (in_open ? NULL : conn, @@ -8495,7 +8505,8 @@ remoteIOEventLoop(virConnectPtr conn, 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) { @@ -8639,7 +8650,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 e8fc565..a1d9ed0 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
* src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report failures on pipe used for wakeup. Reported by Chris Lalancette. --- I think this is what you had in mind for additional error checking. However, in practice, will one-byte reads and writes on fd's created by pipe() ever encounter any errors? src/remote/remote_driver.c | 27 +++++++++++++++++++++++---- 1 files changed, 23 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 57cf8a2..e4a4b73 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8505,8 +8505,12 @@ remoteIOEventLoop(virConnectPtr conn, if (fds[1].revents) { DEBUG0("Woken up from poll by other thread"); - ignore_value (saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + if (saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)) + != sizeof(ignore)) { + virReportSystemError(errno ? errno : 0, + "%s", _("read on wakeup fd failed")); + goto error; + } } if (ret < 0) { @@ -8642,6 +8646,7 @@ remoteIO(virConnectPtr conn, /* Stick ourselves on the end of the wait queue */ struct remote_thread_call *tmp = priv->waitDispatch; char ignore = 1; + ssize_t s; while (tmp && tmp->next) tmp = tmp->next; if (tmp) @@ -8649,8 +8654,22 @@ remoteIO(virConnectPtr conn, else priv->waitDispatch = thiscall; - /* Force other thread to wakup from poll */ - ignore_value (safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); + /* Force other thread to wakeup from poll */ + s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + if (s < 0) { + errorf(flags & REMOTE_CALL_IN_OPEN ? NULL : conn, + VIR_ERR_INTERNAL_ERROR, + _("failed to wake up polling thread: %s"), + strerror(errno)); + VIR_FREE(thiscall); + return -1; + } else if (s != sizeof(ignore)) { + errorf(flags & REMOTE_CALL_IN_OPEN ? NULL : conn, + VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to wake up polling thread")); + VIR_FREE(thiscall); + return -1; + } DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */ -- 1.6.6.1
On 03/09/2010 03:25 PM, Eric Blake wrote:
* src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report failures on pipe used for wakeup. Reported by Chris Lalancette. ---
I think this is what you had in mind for additional error checking. However, in practice, will one-byte reads and writes on fd's created by pipe() ever encounter any errors?
+ s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + if (s < 0) { + errorf(flags & REMOTE_CALL_IN_OPEN ? NULL : conn, + VIR_ERR_INTERNAL_ERROR, + _("failed to wake up polling thread: %s"), + strerror(errno));
Of course, this patch (and also 1/2) fails 'make syntax-check', thanks to strerror. Yet another respin coming. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
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. --- Diff from last attempt: use virStrerror instead of raw strerror. daemon/event.c | 5 +++-- src/remote/remote_driver.c | 19 +++++++++++++++---- src/util/util.h | 9 ++++++--- 3 files changed, 24 insertions(+), 9 deletions(-) diff --git a/daemon/event.c b/daemon/event.c index 2218a3e..36e9dd3 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 11513bd..0ef0416 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -7873,7 +7873,11 @@ remoteIOReadBuffer(virConnectPtr conn, 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, @@ -7882,7 +7886,13 @@ remoteIOReadBuffer(virConnectPtr conn, } else { char errout[1024] = "\0"; if (priv->errfd != -1) { - saferead(priv->errfd, errout, sizeof(errout)); + if (saferead(priv->errfd, errout, sizeof(errout)) < 0) { + errorf (in_open ? NULL : conn, + VIR_ERR_SYSTEM_ERROR, + _("server closed connection: %s"), + virStrerror(errno, errout, sizeof errout)); + return -1; + } } errorf (in_open ? NULL : conn, @@ -8495,7 +8505,8 @@ remoteIOEventLoop(virConnectPtr conn, 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) { @@ -8639,7 +8650,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 e8fc565..a1d9ed0 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
* src/remote/remote_driver.c (remoteIO, remoteIOEventLoop): Report failures on pipe used for wakeup. Reported by Chris Lalancette. --- src/remote/remote_driver.c | 28 ++++++++++++++++++++++++---- 1 files changed, 24 insertions(+), 4 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 0ef0416..bba6faf 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8505,8 +8505,12 @@ remoteIOEventLoop(virConnectPtr conn, if (fds[1].revents) { DEBUG0("Woken up from poll by other thread"); - ignore_value (saferead(priv->wakeupReadFD, &ignore, - sizeof(ignore))); + if (saferead(priv->wakeupReadFD, &ignore, sizeof(ignore)) + != sizeof(ignore)) { + virReportSystemError(errno ? errno : 0, + "%s", _("read on wakeup fd failed")); + goto error; + } } if (ret < 0) { @@ -8642,6 +8646,7 @@ remoteIO(virConnectPtr conn, /* Stick ourselves on the end of the wait queue */ struct remote_thread_call *tmp = priv->waitDispatch; char ignore = 1; + ssize_t s; while (tmp && tmp->next) tmp = tmp->next; if (tmp) @@ -8649,8 +8654,23 @@ remoteIO(virConnectPtr conn, else priv->waitDispatch = thiscall; - /* Force other thread to wakup from poll */ - ignore_value (safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore))); + /* Force other thread to wakeup from poll */ + s = safewrite(priv->wakeupSendFD, &ignore, sizeof(ignore)); + if (s < 0) { + char errout[1024]; + errorf(flags & REMOTE_CALL_IN_OPEN ? NULL : conn, + VIR_ERR_INTERNAL_ERROR, + _("failed to wake up polling thread: %s"), + virStrerror(errno, errout, sizeof errout)); + VIR_FREE(thiscall); + return -1; + } else if (s != sizeof(ignore)) { + errorf(flags & REMOTE_CALL_IN_OPEN ? NULL : conn, + VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to wake up polling thread")); + VIR_FREE(thiscall); + return -1; + } DEBUG("Going to sleep %d %p %p", thiscall->proc_nr, priv->waitDispatch, thiscall); /* Go to sleep while other thread is working... */ -- 1.6.6.1
participants (2)
-
Chris Lalancette -
Eric Blake