[libvirt] [PATCH] replace last instances of close()
Stefan Berger
stefanb at linux.vnet.ibm.com
Tue Nov 16 13:09:35 UTC 2010
On 11/10/2010 07:43 AM, Stefan Berger wrote:
> On 11/10/2010 05:41 AM, Daniel P. Berrange wrote:
>> On Tue, Nov 09, 2010 at 07:43:23PM -0500, Stefan Berger wrote:
>>> I am replacing the last instances of close() I found with VIR_CLOSE() /
>>> VIR_FORCE_CLOSE respectively.
>>>
>>> The first patch of virsh I missed out on previously.
>>>
>>> The 2nd patch I had left out intentionally to look at it more
>>> carefully:
>>> The 'closed' variable could be easily removed since it wasn't used
>>> anywhere else. The possible race condition that could result from the
>>> filedescriptor being closed and not set to -1 (and possibly let us
>>> write
>>> into 'something' totally different if the fd was allocated by another
>>> thread) seems to be prevented by the qemuMonitorLock() already placed
>>> around the code that reads from or writes to the fd. So the change of
>>> this code as shown in the patch should not have any side-effects.
>>>
>>> Signed-off-by: Stefan Berger<stefanb at us.ibm.com>
>>> --- libvirt-acl.orig/src/qemu/qemu_monitor.c
>>> +++ libvirt-acl/src/qemu/qemu_monitor.c
>>> @@ -73,8 +73,6 @@ struct _qemuMonitor {
>>>
>>> /* If the monitor EOF callback is currently active (stops more
>>> commands being run) */
>>> unsigned eofcb: 1;
>>> - /* If the monitor is in process of shutting down */
>>> - unsigned closed: 1;
>>>
>>> unsigned json: 1;
>>> };
>>> @@ -692,17 +690,11 @@ void qemuMonitorClose(qemuMonitorPtr mon
>>> VIR_DEBUG("mon=%p", mon);
>>>
>>> qemuMonitorLock(mon);
>>> - if (!mon->closed) {
>>> +
>>> + if (mon->fd>= 0) {
>>> if (mon->watch)
>>> virEventRemoveHandle(mon->watch);
>>> - if (mon->fd != -1)
>>> - close(mon->fd);
>>> - /* NB: ordinarily one might immediately set mon->watch to -1
>>> - * and mon->fd to -1, but there may be a callback active
>>> - * that is still relying on these fields being valid. So
>>> - * we merely close them, but not clear their values and
>>> - * use this explicit 'closed' flag to track this state */
>>> - mon->closed = 1;
>>> + VIR_FORCE_CLOSE(mon->fd);
>>> }
>> Err, the comment you deleted here explains why this change is not
>> safe.
>
> If I looked at the code correctly then this here is the callback:
>
> static void
> qemuMonitorIO(int watch, int fd, int events, void *opaque) {
> qemuMonitorPtr mon = opaque;
> int quit = 0, failed = 0;
>
> qemuMonitorLock(mon);
> qemuMonitorRef(mon);
> #if DEBUG_IO
> VIR_DEBUG("Monitor %p I/O on watch %d fd %d events %d", mon, watch, f
> #endif
>
> if (mon->fd != fd || mon->watch != watch) {
> VIR_ERROR(_("event from unexpected fd %d!=%d / watch %d!=%d"), mo
> failed = 1;
> } else {
> [...]
>
>
> It grabs the lock and then subsequently, with the lock held, calls
> into qemuMonitorIOWrite and qemuMonitorIORead (not shown, this is done
> in the [...]), which then access the mon->fd. Now the above
> VIR_FORCE_CLOSE() function is also being called with the lock held,
> thus serializes the access to the file descriptor (mon->fd). When the
> 'if (mon->fd != fd)' is evaluate (while the lock is held) after the fd
> was closed and now set to -1, then we won't read from a non-open file
> descriptor anymore, which would otherwise occur in the else branch.
> Otherwise, if fd->mon != -1, it would do the reads and writes as
> intended and the above close could not happen while it is doing that.
>
> When I made the above change I first introduced another lock
> 'mon->monFDLock' to serialize the access to the fd, but then I saw
> that the qemuMonitorLock() was already doing that as well, so I ended
> up removing it again entirely. Well, to me this was convincing so that
> the above changes seemed 'safe'. Actually, I think the previous code
> could write and read to a file descriptor that doesn't communicate
> with the monitor anymore but may belong to another thread now (since
> the mon->fd wasn't set to -1, but kept as-is). So, to me this is
> actually an improvement.
>
> Regards,
> Stefan
>
Did I convince you?
Regards,
Stefan
More information about the libvir-list
mailing list