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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list