[libvirt] [PATCHv2] util: ensure safe{read, write, zero} return is checked

[Eric Blake]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 323 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20100309/dac924f8/attachment-0001.sig>