[PATCH 3/3] qemu: Update paused reason on unexpected 'STOP' event

[Michal Privoznik]

On 3/18/21 6:40 PM, Peter Krempa wrote:
> The qemu STOP event doesn't really report why the VM was stopped. In
> certain cases we do expect this by storing the expectation in the
> private data.
> 
> In cases we've encountered an unexpected STOP event we can offload to
> the event thread a job to refresh the state using 'query-status'.
> 
> For all other paused reasons which are expected we keep the original
> logic.
> 
> Signed-off-by: Peter Krempa <pkrempa at redhat.com>
> ---
>   src/qemu/qemu_domain.c  |  1 +
>   src/qemu/qemu_domain.h  |  1 +
>   src/qemu/qemu_driver.c  | 37 +++++++++++++++++++++++++++++++++++++
>   src/qemu/qemu_process.c | 22 +++++++++++++++++++---
>   4 files changed, 58 insertions(+), 3 deletions(-)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index dde9ba92c6..106a292ef3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11049,6 +11049,7 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
>           virObjectUnref(event->data);
>           break;
>       case QEMU_PROCESS_EVENT_PR_DISCONNECT:
> +    case QEMU_PROCESS_EVENT_STOP:
>       case QEMU_PROCESS_EVENT_LAST:
>           break;
>       }
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 949307229b..b057e94df0 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -441,6 +441,7 @@ typedef enum {
>       QEMU_PROCESS_EVENT_PR_DISCONNECT,
>       QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED,
>       QEMU_PROCESS_EVENT_GUEST_CRASHLOADED,
> +    QEMU_PROCESS_EVENT_STOP,
> 
>       QEMU_PROCESS_EVENT_LAST
>   } qemuProcessEventType;
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index f3f8caab44..53be363338 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -4316,6 +4316,40 @@ processGuestCrashloadedEvent(virQEMUDriverPtr driver,
>   }
> 
> 
> +static void
> +processGuestStopEvent(virDomainObjPtr vm)
> +{
> +    qemuDomainObjPrivatePtr priv = vm->privateData;
> +    virObjectEventPtr event = NULL;
> +    virDomainPausedReason reason = VIR_DOMAIN_PAUSED_UNKNOWN;
> +
> +    if (qemuDomainObjBeginJob(priv->driver, vm, QEMU_JOB_MODIFY) == 0) {
> +        bool running;
> +        int rc;

While @vm is locked when this function is called it was unlocked for a 
brief moment. Therefore, another thread might have changed its state. 
Hence, virDomainObjIsActive() is a must here.

> +
> +        qemuDomainObjEnterMonitor(priv->driver, vm);
> +
> +        rc = qemuMonitorGetStatus(priv->mon, &running, &reason);
> +
> +        /* update reason only on successful state update */
> +        if (qemuDomainObjExitMonitor(priv->driver, vm) < 0)
> +            rc = -1;
> +
> +        qemuDomainObjEndJob(priv->driver, vm);
> +
> +        if (rc < 0)
> +            return;
> +    }
> +
> +    virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason);
> +

Again, another thread might have just resumed vCPUS. Setting this here 
might lead to wrong state. What if we called SetState() iff !running && 
reason? I mean, SetState() was called from the in-loop handler already 
and this is here only to report reason (if any).

> +    event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED,
> +                                              qemuDomainPausedReasonToSuspendedEvent(reason));
> +
> +    virObjectEventStateQueue(priv->driver->domainEventState, event);
> +}
> +
> +
>   static void qemuProcessEventHandler(void *data, void *opaque)
>   {
>       struct qemuProcessEvent *processEvent = data;
> @@ -4365,6 +4399,9 @@ static void qemuProcessEventHandler(void *data, void *opaque)
>       case QEMU_PROCESS_EVENT_GUEST_CRASHLOADED:
>           processGuestCrashloadedEvent(driver, vm);
>           break;
> +    case QEMU_PROCESS_EVENT_STOP:
> +        processGuestStopEvent(vm);
> +        break;
>       case QEMU_PROCESS_EVENT_LAST:
>           break;
>       }
> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
> index 5f31260221..7003c92046 100644
> --- a/src/qemu/qemu_process.c
> +++ b/src/qemu/qemu_process.c
> @@ -689,10 +689,26 @@ qemuProcessHandleStop(qemuMonitorPtr mon G_GNUC_UNUSED,
>           if (priv->signalStop)
>               virDomainObjBroadcast(vm);
> 
> +        /* In case of VIR_DOMAIN_PAUSED_UNKNOWN we'll update it later, but we
> +         * must set the state to PAUSED right away. We delay the event until
> +         * the update. */
>           virDomainObjSetState(vm, VIR_DOMAIN_PAUSED, reason);
> -        event = virDomainEventLifecycleNewFromObj(vm,
> -                                                  VIR_DOMAIN_EVENT_SUSPENDED,
> -                                                  detail);
> +        if (reason == VIR_DOMAIN_PAUSED_UNKNOWN) {
> +            /* offload unknown state to thread updating state */
> +            struct qemuProcessEvent *processEvent = g_new0(struct qemuProcessEvent, 1);
> +
> +            processEvent->eventType = QEMU_PROCESS_EVENT_STOP;
> +            processEvent->vm = virObjectRef(vm);
> +
> +            if (virThreadPoolSendJob(driver->workerPool, 0, processEvent) < 0) {
> +                virObjectUnref(vm);
> +                qemuProcessEventFree(processEvent);
> +            }
> +        } else {
> +            event = virDomainEventLifecycleNewFromObj(vm,
> +                                                      VIR_DOMAIN_EVENT_SUSPENDED,
> +                                                      detail);
> +        }
> 
>           VIR_FREE(priv->lockState);
>           if (virDomainLockProcessPause(driver->lockManager, vm, &priv->lockState) < 0)
> 

Michal