Re: [libvirt] [PATCH 3/3] qemu: don't duplicate suspended events and state changes
On 10/17/18 4:58 AM, Nikolay Shirokovskiy wrote:
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.
>
>
Placed pkrempa on the CC line to hopefully get his feedback in order to
either move this along or require some rework due to the change possibly
generating a lifecycle event now. It's not clear from the referenced
commit why no event was desired and I'd prefer Peter's input just in
case there's some particular reason that wasn't immediately evident or
widely known...
Tks -
John
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)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;
>> - }
>>
>
> [...]
>