On Wed, Nov 30, 2011 at 11:54:55 +0000, Daniel P. Berrange wrote:
> On Wed, Nov 30, 2011 at 11:48:16AM +0100, Jiri Denemark wrote:
> static bool
> vshDeinit(vshControl *ctl)
> {
> ctl->quit = true;
> vshReadlineDeinit(ctl);
> vshCloseLogFile(ctl);
> VIR_FREE(ctl->name);
> if (ctl->conn) {
> int ret;
> if ((ret = virConnectClose(ctl->conn)) != 0) {
> vshError(ctl, _("Failed to disconnect from the hypervisor, %d leaked reference(s)"), ret);
> }
> }
> virResetLastError();
>
> if (ctl->eventLoopStarted) {
> /* HACK: Add a dummy timeout to break event loop */
> int timer = virEventAddTimeout(-1, NULL, NULL, NULL);
>
>
> Now look at the event loop thread:
>
> static void
> vshEventLoop(void *opaque)
> {
> vshControl *ctl = opaque;
>
> while (!ctl->quit) {
> if (virEventRunDefaultImpl() < 0) {
> virshReportError(ctl);
> }
> }
> }
>
> Neither the read of ctl->quit, nor the write of ctl->quit are protected
> by any locking. While you have assigned to ctl->quit before adding the
> dummy function, I'm not convinced that is sufficient, because in the
> absence of any thread synchronization point, the compiler is free to
> reorder your assignment to occur later. In addition, the data is not
> neccessarily coherant across threads. IMHO, the read/write of ctl->quit
> needs to be protected by a mutex.
Yeah, you're right. I seem to keep forgetting about this memory barrier "side
effect" of mutexes, which in fact is the only thing we need here :-)
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list