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/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;
So originally this was passed the 'dmn' pointer ...
> -
> - VIR_DEBUG("Begin stop dmn=%p", dmn);
> -
> - ignore_value(virStateStop());
> -
> - VIR_DEBUG("Completed stop dmn=%p", dmn);
> -
... but it was never touched except for logging ...
> - /* Exit daemon cleanly */
> - virNetDaemonQuit(dmn);
... and then using a self-locking API.
> -}
> -
> -
> -/* We do this in a thread to not block the main loop */
> -static void daemonStop(virNetDaemon *dmn)
> -{
> - virThread *thr;
> - virObjectRef(dmn);
All of the above while having a reference.
> -
> - thr = g_new0(virThread, 1);
> -
> - if (virThreadCreateFull(thr, true,
> - daemonStopWorker,
> - "daemon-stop", false, dmn) < 0) {
So this could make it async without worries ...
> - virObjectUnref(dmn);
> - g_free(thr);
> - return;
> - }
> -
> - virNetDaemonSetStateStopWorkerThread(dmn, &thr);
... and without touching 'dmn' while unlocked.
> -}
> -
> -
> 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,
> 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)
> }
[Re-organized hunks for explanatory reasons]
>
> +
> +
> +void virNetDaemonPreserve(virNetDaemon *dmn)
> +{
> + VIR_LOCK_GUARD lock = virObjectLockGuard(dmn);
But in the new code, we lock 'dmn' ...
> +
> + if (!dmn->shutdownPreserveCb ||
> + dmn->shutdownPreserveThread)
... so that we can safely access the properties of virNetDaemon ..
> + return;
> +
> + virObjectRef(dmn);
> + dmn->shutdownPreserveThread = g_new0(virThread, 1);
> +
> + if (virThreadCreateFull(dmn->shutdownPreserveThread, true,
> + virNetDaemonPreserveWorker,
> + "daemon-stop", false, dmn) < 0) {
.... but once you make it async here ...
> + virObjectUnref(dmn);
> + g_clear_pointer(&dmn->shutdownPreserveThread, g_free);
> + return;
> + }
... and the context of this function finishes right away, 'dmn' will be
unlocked ...
> +}
> +
> +
>
> +static void virNetDaemonPreserveWorker(void *opaque)
> +{
> + virNetDaemon *dmn = opaque;
> +
> + VIR_DEBUG("Begin stop dmn=%p", dmn);
> +
> + dmn->shutdownPreserveCb();
... but here we access it's props.
> +
> + VIR_DEBUG("Completed stop dmn=%p", dmn);
> +
> + virNetDaemonQuit(dmn);
... now this uses self locking APIs once more.
> + virObjectUnref(dmn);
> +}
> 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;
And this setter technically makes it non-immutable.
While it most likely will not be a problem I don't think we should
access this function pointer outside of a lock. That is unless you
document it as being/make it immutable.
Upcoming patches also add code where virNetDaemonPreserveWorker will
access dmn->quit which definitely isn't immutable so it's likely a more
complete solution will be needed.
