[PATCH 0/2] remote_daemon: Set shutdown callbacks only after init is done
*** BLURB HERE *** Michal Prívozník (2): remote_daemon: Set shutdown callbacks only after init is done Revert "qemu: Avoid crash in qemuStateShutdownPrepare() and qemuStateShutdownWait()" src/qemu/qemu_driver.c | 6 ------ src/remote/remote_daemon.c | 7 ++++--- 2 files changed, 4 insertions(+), 9 deletions(-) -- 2.32.0
The initialization of drivers happens in a separate thread. However, the main thread continues initialization and sets shutdown callbacks (virStateShutdownPrepare() and virStateShutdownWait()) even though the driver init thread is still running. This is dangerous because if the daemon decides to quit early (e.g. because SIGINT was delivered) the shutdownPrepare and shutdownWait callback are called over partially init drivers. Set callbacks only after all drivers were initialized. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/218 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2027400 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index de43a54c2e..4e10f3ad23 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -626,6 +626,10 @@ static void daemonRunStateInit(void *opaque) driversInitialized = true; + virNetDaemonSetShutdownCallbacks(dmn, + virStateShutdownPrepare, + virStateShutdownWait); + /* Tie the non-privileged daemons to the session/shutdown lifecycle */ if (!virNetDaemonIsPrivileged(dmn)) { @@ -1214,9 +1218,6 @@ int main(int argc, char **argv) { #endif /* Run event loop. */ - virNetDaemonSetShutdownCallbacks(dmn, - virStateShutdownPrepare, - virStateShutdownWait); virNetDaemonRun(dmn); ret = 0; -- 2.32.0
On Thu, Dec 09, 2021 at 15:43:55 +0100, Michal Privoznik wrote:
The initialization of drivers happens in a separate thread. However, the main thread continues initialization and sets shutdown callbacks (virStateShutdownPrepare() and virStateShutdownWait()) even though the driver init thread is still running. This is dangerous because if the daemon decides to quit early (e.g. because SIGINT was delivered) the shutdownPrepare and shutdownWait callback are called over partially init drivers.
Set callbacks only after all drivers were initialized.
Resolves: https://gitlab.com/libvirt/libvirt/-/issues/218 Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2027400
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/remote/remote_daemon.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c index de43a54c2e..4e10f3ad23 100644 --- a/src/remote/remote_daemon.c +++ b/src/remote/remote_daemon.c @@ -626,6 +626,10 @@ static void daemonRunStateInit(void *opaque)
driversInitialized = true;
+ virNetDaemonSetShutdownCallbacks(dmn, + virStateShutdownPrepare, + virStateShutdownWait); +
Okay so this placement ensures that the state shutdown code will only ever be executed if the state was already initialized ...
/* Tie the non-privileged daemons to the session/shutdown lifecycle */ if (!virNetDaemonIsPrivileged(dmn)) {
@@ -1214,9 +1218,6 @@ int main(int argc, char **argv) { #endif
/* Run event loop. */ - virNetDaemonSetShutdownCallbacks(dmn, - virStateShutdownPrepare, - virStateShutdownWait);
... which wasn't always true here.
virNetDaemonRun(dmn);
ret = 0;
So at this point it's still possible that the daemon will quit without the callbacks being called, but tat was possible even before. Based on the above and the fact that I wasn't able to reproduce the crash: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
This reverts commit 69977ff10560a80bcf5bf93f1a3f819a2d1623ca. After previous commit it's no longer possible that QEMU driver is not initialized in qemuStateShutdownPrepare() nor qemuStateShutdownWait(). Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8093b8f69b..2b1a34fafb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1034,9 +1034,6 @@ qemuStateStop(void) static int qemuStateShutdownPrepare(void) { - if (!qemu_driver) - return 0; - virThreadPoolStop(qemu_driver->workerPool); return 0; } @@ -1056,9 +1053,6 @@ qemuDomainObjStopWorkerIter(virDomainObj *vm, static int qemuStateShutdownWait(void) { - if (!qemu_driver) - return 0; - virDomainObjListForEach(qemu_driver->domains, false, qemuDomainObjStopWorkerIter, NULL); virThreadPoolDrain(qemu_driver->workerPool); -- 2.32.0
On Thu, Dec 09, 2021 at 15:43:56 +0100, Michal Privoznik wrote:
This reverts commit 69977ff10560a80bcf5bf93f1a3f819a2d1623ca.
After previous commit it's no longer possible that QEMU driver is not initialized in qemuStateShutdownPrepare() nor qemuStateShutdownWait().
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 6 ------ 1 file changed, 6 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
participants (2)
-
Michal Privoznik -
Peter Krempa