Re: [PATCH 2/2] qemu: Use virEventThreadStop() in qemuProcessStop()

On 7/25/24 14:18, Daniel P. Berrangé wrote: > On Thu, Jul 25, 2024 at 12:57:59PM +0200, Michal Privoznik wrote: >> Currently, qemuProcessStop() unlocks given domain object right in >> the middle of cleanup process. This is dangerous because there >> might be another thread which is executing virDomainObjListAdd(). >> And since the domain object is on the list of domain objects AND >> by the time qemuProcessStop() unlocks it the object is also >> marked as inactive, the other thread acquires the lock and >> switches vm->def pointer. >> >> The unlocking of domain object is needed though, to allow even >> processing thread finish its queue. Well, the processing can be >> done before any cleanup is attempted. >> >> Therefore, use freshly introduced virEventThreadStop() to join >> the event thread and drop lock/unlock from the middle of >> qemuProcessStop(). >> >> Fixes: 3865410e7f67ca4ec66e9a905e75f452762a97f0 >> Resolves: https://issues.redhat.com/browse/RHEL-49607 >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_process.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index 6255ba92e7..0869307a90 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -8615,6 +8615,14 @@ void qemuProcessStop(virQEMUDriver *driver, >> * reporting so we don't squash a legit error. */ >> virErrorPreserveLast(&orig_err); >> >> + /* By unlocking the domain object the events processing thread is >> + * allowed to finish its job. */ >> + if (priv->eventThread) { >> + virObjectUnlock(vm); >> + virEventThreadStop(priv->eventThread); >> + virObjectLock(vm); >> + } >> + > > I wonder if this is a little too early. > > We don't call qemuMOnitorClose until a short while later, > just after we have done qemuProcessKill. > > If we stop the event loop here, then I worry that we're > at risk of missing some final monitor events that might > be desirable ? eg the final lifecycle events indicating > that we're stopping/stopped ? But can we close the monitor without a job? And IIUC, we shouldn't try to stop the event loop thread with a job held (as those callbacks try to acquire _MODIFY job on their own).