[libvirt] [PATCH 3/3] qemu: don't duplicate suspended events and state changes

[Nikolay Shirokovskiy]

On 16.10.2018 21:46, John Ferlan wrote:
> 
> $SUBJ:
> 
> s/don't/Don't
> 
> On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
>> Now when STOP event handler has correct both suspended event reason
>> and paused state reason let's wipe out duplicated event sending and
>> state changed in/after qemuProcessStopCPUs.
> 
> Since the STOP event handler can use the pausedReason as sent to
> qemuProcessStopCPUs, we no longer need to send duplicate suspended
> lifecycle events because we know what caused the stop along with extra
> details. This processing allows us to also remove the duplicated state
> change from qemuProcessStopCPUs.
> 
> 
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>> ---
>>  src/qemu/qemu_driver.c    | 26 +++-----------------------
>>  src/qemu/qemu_migration.c | 42 ++++++------------------------------------
>>  src/qemu/qemu_migration.h |  4 ----
>>  src/qemu/qemu_process.c   | 13 +++++++------
>>  4 files changed, 16 insertions(+), 69 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index a52e249..7e08768 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -1792,10 +1792,8 @@ static int qemuDomainSuspend(virDomainPtr dom)
>>      virQEMUDriverPtr driver = dom->conn->privateData;
>>      virDomainObjPtr vm;
>>      int ret = -1;
>> -    virObjectEventPtr event = NULL;
>>      qemuDomainObjPrivatePtr priv;
>>      virDomainPausedReason reason;
>> -    int eventDetail;
>>      int state;
>>      virQEMUDriverConfigPtr cfg = NULL;
>>  
>> @@ -1814,16 +1812,12 @@ static int qemuDomainSuspend(virDomainPtr dom)
>>      if (virDomainObjCheckActive(vm) < 0)
>>          goto endjob;
>>  
>> -    if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT) {
>> +    if (priv->job.asyncJob == QEMU_ASYNC_JOB_MIGRATION_OUT)
>>          reason = VIR_DOMAIN_PAUSED_MIGRATION;
>> -        eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED;
>> -    } else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT) {
>> +    else if (priv->job.asyncJob == QEMU_ASYNC_JOB_SNAPSHOT)
>>          reason = VIR_DOMAIN_PAUSED_SNAPSHOT;
>> -        eventDetail = -1; /* don't create lifecycle events when doing snapshot */
> 
> So with these changes how do we handle this special case? See commit
> f569b87f5 when this was introduced.
> 
> Do we need to adjust qemuProcessHandleStop to not generate the event
> when "reason == VIR_DOMAIN_PAUSED_SNAPSHOT && priv->job.asyncJob ==
> QEMU_ASYNC_JOB_SNAPSHOT"?

Well we had lifecycle event anyway because of stop handler so I decided
to keep sending it. However I'm not sure why it was not sent in qemuDomainSuspend
originally. For example for migration we do send event. Looks like this
event does not hurt anyone if it survives up to now.

I'm ok with commit message to if the case.

Nikolay

> 
> The rest is OK - just need your thoughts on this particular case for the
> R-By.
> 
> John
> 
>> -    } else {
>> +    else
>>          reason = VIR_DOMAIN_PAUSED_USER;
>> -        eventDetail = VIR_DOMAIN_EVENT_SUSPENDED_PAUSED;
>> -    }
>>  
> 
> [...]
>