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(a)redhat.com +1-801-349-2682
Libvirt virtualization library
http://libvirt.org