On Wed, Aug 07, 2024 at 14:44:56 +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 <[email protected]>
> ---
I've requested that the commit message carries an explanation why you
can change the code to contradict what the coment, which is being
deleted below states.
> src/qemu/qemu_process.c | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index cec739c984..a07ce50fbf 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -8692,6 +8692,16 @@ void qemuProcessStop(virQEMUDriver *driver,
> VIR_QEMU_PROCESS_KILL_FORCE|
> VIR_QEMU_PROCESS_KILL_NOCHECK));
>
> + /* By unlocking the domain object the events processing thread is allowed
> + * to finish its job. Unlocking must happen before resetting vm->def->id
> as
> + * the global domain object list code depends on it (and it can't
> actually
> + * check 'priv->beingDestroyed as that's private). */
> + if (priv->eventThread) {
I've also requested to explicitly set 'priv->beingDestroyed' before
unlocking as qemuProcessStop may be called from code paths which don't
do that.
With both of the above addressed as I've requested in previous review:
Reviewed-by: Peter Krempa <[email protected]>
