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

[Daniel P. Berrange]

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.

Regards,
Daniel
-- 
|: Red Hat, Engineering, London    -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :|
|: http://autobuild.org        -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|