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