[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] replace last instances of close()



On 11/16/2010 06:20 AM, Stefan Berger wrote:
>>> Did I convince you?
>> Yes&  no. On the surface it looks ok, but I'm still a little worried
>> about
>> the implications, because I put this code in there to avoid some nasty
>> re-entrancy problems. I guessing some later re-factoring might have fixed
>> those problems making this code redundant. So I'll ACK for nwo, but if we
>> notice any unusual behaviour with the monitor, then this is the place to
>> look...
>>
> The re-entrancy that was previously avoided with the 'closed' flag is
> now checked with the file descriptor being >= 0, both with the lock
> being held, so no concurrency is possible.
> 
> The most dangerous thing IMO is that someone changes the locking later
> on for whatever reason. There really should be a comment on the
> functions reading and writing from/to the mon->fd that they need to be
> called with that lock held. Also that the 'mon-fd' is protected by the
> existing lock may be worth a comment.
> 
> I'd push unless you want me to add comments to the file descriptor and
> functions requiring the lock being held.

We need the comments; no one will remember to dig up this email
conversation, but in-code documentation will remind us to be careful.

-- 
Eric Blake   eblake redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]