On Wed, Jan 08, 2025 at 19:42:54 +0000, Daniel P. Berrangé wrote:
> Currently the remote daemon code is responsible for calling virStateStop
> in a background thread. The virNetDaemon code wants to synchronize with
> this during shutdown, however, so the virThreadPtr must be passed over.
>
> Even the limited synchronization done currently, however, is flawed and
> to fix this requires the virNetDaemon code to be responsible for calling
> virStateStop in a thread more directly.
>
> Thus the logic is moved over into virStateStop via a further callback
> to be registered by the remote daemon.
>
> Signed-off-by: Daniel P. Berrangé <berrange@xxxxxxxxxx>
> ---
> src/libvirt_remote.syms | 2 +-
> src/remote/remote_daemon.c | 40 ++------------------------
> src/rpc/virnetdaemon.c | 58 ++++++++++++++++++++++++++++----------
> src/rpc/virnetdaemon.h | 20 +++++++++++--
> 4 files changed, 64 insertions(+), 56 deletions(-)
>
> diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
> index f0f90815cf..7e87b0bd2a 100644
> --- a/src/libvirt_remote.syms
> +++ b/src/libvirt_remote.syms
> @@ -90,12 +90,12 @@ virNetDaemonIsPrivileged;
> virNetDaemonNew;
> virNetDaemonNewPostExecRestart;
> virNetDaemonPreExecRestart;
> +virNetDaemonPreserve;
> virNetDaemonQuit;
> virNetDaemonQuitExecRestart;
> virNetDaemonRemoveShutdownInhibition;
> virNetDaemonRun;
> virNetDaemonSetShutdownCallbacks;
> -virNetDaemonSetStateStopWorkerThread;
> virNetDaemonUpdateServices;
>
>
> diff --git a/src/remote/remote_daemon.c b/src/remote/remote_daemon.c
> index c4b930cb70..bf91ee5772 100644
> --- a/src/remote/remote_daemon.c
> +++ b/src/remote/remote_daemon.c
> @@ -514,41 +514,6 @@ static void daemonInhibitCallback(bool inhibit, void *opaque)
> static GDBusConnection *sessionBus;
> static GDBusConnection *systemBus;
>
> -static void daemonStopWorker(void *opaque)
> -{
> - virNetDaemon *dmn = opaque;
> -
> - VIR_DEBUG("Begin stop dmn=%p", dmn);
> -
> - ignore_value(virStateStop());
> -
> - VIR_DEBUG("Completed stop dmn=%p", dmn);
> -
> - /* Exit daemon cleanly */
> - virNetDaemonQuit(dmn);
> -}
> -
> -
> -/* We do this in a thread to not block the main loop */
> -static void daemonStop(virNetDaemon *dmn)
> -{
> - virThread *thr;
> - virObjectRef(dmn);
> -
> - thr = g_new0(virThread, 1);
> -
> - if (virThreadCreateFull(thr, true,
> - daemonStopWorker,
> - "daemon-stop", false, dmn) < 0) {
> - virObjectUnref(dmn);
> - g_free(thr);
> - return;
> - }
> -
> - virNetDaemonSetStateStopWorkerThread(dmn, &thr);
> -}
> -
> -
> static GDBusMessage *
> handleSessionMessageFunc(GDBusConnection *connection G_GNUC_UNUSED,
> GDBusMessage *message,
> @@ -562,7 +527,7 @@ handleSessionMessageFunc(GDBusConnection *connection G_GNUC_UNUSED,
> if (virGDBusMessageIsSignal(message,
> "org.freedesktop.DBus.Local",
> "Disconnected"))
> - daemonStop(dmn);
> + virNetDaemonPreserve(dmn);
>
> return message;
> }
> @@ -581,7 +546,7 @@ handleSystemMessageFunc(GDBusConnection *connection G_GNUC_UNUSED,
>
> VIR_DEBUG("dmn=%p", dmn);
>
> - daemonStop(dmn);
> + virNetDaemonPreserve(dmn);
> }
>
>
> @@ -625,6 +590,7 @@ static void daemonRunStateInit(void *opaque)
> g_atomic_int_set(&driversInitialized, 1);
>
> virNetDaemonSetShutdownCallbacks(dmn,
> + virStateStop,
The name 'virStateStop' is starting to be confusing in the context it's
being used now. I suggest you at least document what the expectations
from the hypervisor drivers are? Or better when it's invoked.
> virStateShutdownPrepare,
> virStateShutdownWait);
>
> diff --git a/src/rpc/virnetdaemon.c b/src/rpc/virnetdaemon.c
> index bb7d2ff9a0..19b19ff834 100644
> --- a/src/rpc/virnetdaemon.c
> +++ b/src/rpc/virnetdaemon.c
> @@ -65,9 +65,10 @@ struct _virNetDaemon {
> GHashTable *servers;
> virJSONValue *srvObject;
>
> + virNetDaemonShutdownCallback shutdownPreserveCb;
> virNetDaemonShutdownCallback shutdownPrepareCb;
> virNetDaemonShutdownCallback shutdownWaitCb;
> - virThread *stateStopThread;
> + virThread *shutdownPreserveThread;
> int finishTimer;
> bool quit;
> bool finished;
> @@ -107,7 +108,7 @@ virNetDaemonDispose(void *obj)
> virEventRemoveHandle(dmn->sigwatch);
> #endif /* !WIN32 */
>
> - g_free(dmn->stateStopThread);
> + g_free(dmn->shutdownPreserveThread);
>
> g_clear_pointer(&dmn->servers, g_hash_table_unref);
>
> @@ -705,8 +706,8 @@ daemonShutdownWait(void *opaque)
>
> virHashForEach(dmn->servers, daemonServerShutdownWait, NULL);
> if (!dmn->shutdownWaitCb || dmn->shutdownWaitCb() >= 0) {
> - if (dmn->stateStopThread)
> - virThreadJoin(dmn->stateStopThread);
> + if (dmn->shutdownPreserveThread)
> + virThreadJoin(dmn->shutdownPreserveThread);
>
> graceful = true;
> }
> @@ -801,17 +802,6 @@ virNetDaemonRun(virNetDaemon *dmn)
> }
>
>
> -void
> -virNetDaemonSetStateStopWorkerThread(virNetDaemon *dmn,
> - virThread **thr)
> -{
> - VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);
> -
> - VIR_DEBUG("Setting state stop worker thread on dmn=%p to thr=%p", dmn, thr);
> - dmn->stateStopThread = g_steal_pointer(thr);
> -}
> -
> -
> void
> virNetDaemonQuit(virNetDaemon *dmn)
> {
> @@ -833,6 +823,42 @@ virNetDaemonQuitExecRestart(virNetDaemon *dmn)
> }
>
>
> +static void virNetDaemonPreserveWorker(void *opaque)
> +{
> + virNetDaemon *dmn = opaque;
> +
> + VIR_DEBUG("Begin stop dmn=%p", dmn);
> +
> + dmn->shutdownPreserveCb();
> +
> + VIR_DEBUG("Completed stop dmn=%p", dmn);
> +
> + virNetDaemonQuit(dmn);
> + virObjectUnref(dmn);
> +}
> +
> +
> +void virNetDaemonPreserve(virNetDaemon *dmn)
> +{
> + VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);
> +
> + if (!dmn->shutdownPreserveCb ||
> + dmn->shutdownPreserveThread)
> + return;
> +
> + virObjectRef(dmn);
> + dmn->shutdownPreserveThread = g_new0(virThread, 1);
> +
> + if (virThreadCreateFull(dmn->shutdownPreserveThread, true,
> + virNetDaemonPreserveWorker,
> + "daemon-stop", false, dmn) < 0) {
> + virObjectUnref(dmn);
> + g_clear_pointer(&dmn->shutdownPreserveThread, g_free);
> + return;
> + }
> +}
Please use the new function header formatting style.
> +
> +
> static int
> daemonServerClose(void *payload,
> const char *key G_GNUC_UNUSED,
> @@ -870,11 +896,13 @@ virNetDaemonHasClients(virNetDaemon *dmn)
>
> void
> virNetDaemonSetShutdownCallbacks(virNetDaemon *dmn,
> + virNetDaemonShutdownCallback preserveCb,
> virNetDaemonShutdownCallback prepareCb,
> virNetDaemonShutdownCallback waitCb)
> {
> VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);
>
> + dmn->shutdownPreserveCb = preserveCb;
> dmn->shutdownPrepareCb = prepareCb;
> dmn->shutdownWaitCb = waitCb;
> }
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
