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@xxxxxxxxxx>
> --- 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list