On Mon, Feb 03, 2025 at 10:52:23 +0100, Peter Krempa wrote:
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@redhat.com> --- 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(-)
[...]
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.
Disregard this paragraph, I didn't notice the lock guard in the patch adding the access to dmn->quit. Thus declaring 'shutdownPreserveCb' to be immutable should be good enough.