
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