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