From: Denis V. Lunev <[email protected]> virObjectEventStateQueueRemote() checks state->timer before taking the state lock. If the last callback is removed in between, virObjectEventStateCleanupTimer() removes the timer and clears the queue, and the event is then queued with no timer to flush it. A callback registered later gets a new timer, but that timer is armed only when a push takes the queue from empty to one event, which never happens again. Neither the stale event nor anything queued after it is dispatched, and no client receives events until the daemon restarts.
A client holding the last callback can trigger this by exiting: its connection drops the callbacks while the hypervisor reports the side effects of the exit, e.g. VNC_DISCONNECTED for a VNC session it had open. Check the timer under the lock. The test removes the last callback while another thread is blocked queueing an event, using a freecb that runs under the state lock, and then checks that a newly registered callback still receives events. Signed-off-by: Denis V. Lunev <[email protected]> --- Seen on a host where a management daemon was the only event consumer: after it was restarted with a VNC session open, libvirtd delivered no domain events to anyone, a fresh 'virsh event --all' included, until libvirtd itself was restarted. Restarting that daemon in a loop with a VM running reproduced it in the 11th round; gdb on libvirtd showed timer == -1 with queue->count == 1 right after the daemon exited. The new test failed 5 of 5 runs without the fix and passed 20 of 20 with it. With the fix the event is simply dropped, so the test does not depend on thread timing. The whole test suite, syntax-check included, passes. object_event.c has had the unlocked check since it was split out of domain_event.c in v1.2.1, and domain_event.c had it before that. src/conf/object_event.c | 5 ++- tests/objecteventtest.c | 92 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 2 deletions(-) diff --git a/src/conf/object_event.c b/src/conf/object_event.c index 5fb96cb7aa..8b0669d660 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -727,13 +727,14 @@ virObjectEventStateQueueRemote(virObjectEventState *state, if (!event) return; + virObjectLock(state); + if (state->timer < 0) { + virObjectUnlock(state); virObjectUnref(event); return; } - virObjectLock(state); - event->remoteID = remoteID; if (virObjectEventQueuePush(state->queue, event) < 0) { VIR_DEBUG("Error adding event to queue"); diff --git a/tests/objecteventtest.c b/tests/objecteventtest.c index 7110f54139..6b43d3c44d 100644 --- a/tests/objecteventtest.c +++ b/tests/objecteventtest.c @@ -22,6 +22,7 @@ #include <unistd.h> #include "testutils.h" +#include "virthread.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -444,6 +445,94 @@ testDomainStartStopEvent(const void *data) return ret; } +typedef struct { + lifecycleEventCounter counter; + virConnectPtr conn; + virThread thread; + bool started; +} queueRaceData; + +static void +queueRaceDefine(void *opaque) +{ + virDomainPtr dom = virDomainDefineXML(opaque, domainDef); + + if (dom) + virDomainFree(dom); +} + +/* Runs under the event state lock while the last callback is removed */ +static void +queueRaceFree(void *opaque) +{ + queueRaceData *race = opaque; + + if (virThreadCreate(&race->thread, true, queueRaceDefine, race->conn) < 0) + return; + race->started = true; + g_usleep(100 * 1000); +} + +static void +queueRaceWatchdog(int timer G_GNUC_UNUSED, + void *opaque G_GNUC_UNUSED) +{ +} + +static int +testDomainEventQueueRace(const void *data) +{ + const objecteventTest *test = data; + queueRaceData race = { .conn = test->conn }; + lifecycleEventCounter counter = { 0 }; + int eventId = VIR_DOMAIN_EVENT_ID_LIFECYCLE; + virDomainPtr dom = NULL; + int timer = -1; + int id; + int ret = -1; + + id = virConnectDomainEventRegisterAny(test->conn, NULL, eventId, + VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb), + &race, queueRaceFree); + if (id < 0) + return -1; + + virConnectDomainEventDeregisterAny(test->conn, id); + if (!race.started) + return -1; + virThreadJoin(&race.thread); + + id = virConnectDomainEventRegisterAny(test->conn, NULL, eventId, + VIR_DOMAIN_EVENT_CALLBACK(&domainLifecycleCb), + &counter, NULL); + if (id < 0) + return -1; + + if (!(dom = virDomainLookupByName(test->conn, "test-domain")) || + virDomainUndefine(dom) < 0) + goto cleanup; + + /* A stuck queue never fires, so do not wait for it forever */ + if ((timer = virEventAddTimeout(1000, queueRaceWatchdog, NULL, NULL)) < 0) + goto cleanup; + + if (virEventRunDefaultImpl() < 0) + goto cleanup; + + if (counter.undefineEvents != 1 || counter.unexpectedEvents > 0) + goto cleanup; + + ret = 0; + cleanup: + if (timer >= 0) + virEventRemoveTimeout(timer); + virConnectDomainEventDeregisterAny(test->conn, id); + if (dom) + virDomainFree(dom); + + return ret; +} + static int testNetworkCreateXML(const void *data) { @@ -804,6 +893,9 @@ mymain(void) ret = EXIT_FAILURE; if (virTestRun("Domain start stop events", testDomainStartStopEvent, &test) < 0) ret = EXIT_FAILURE; + if (virTestRun("Domain event queued while last callback is removed", + testDomainEventQueueRace, &test) < 0) + ret = EXIT_FAILURE; /* Network event tests */ /* Tests requiring the test network not to be set up */ base-commit: 4d514f852c44f05f34e024438a7be004822c059d -- 2.53.0
