[libvirt] [PATCH 1/3] qemu: pass stop reason from qemuProcessStopCPUs to stop handler

[John Ferlan]

On 11/2/18 4:21 AM, Nikolay Shirokovskiy wrote:
> Hi, John!
> 
> Looks like this series is stucked somehow even though there is almost 100% agreement.
> 
> 

Right, but there's been a few events in between that pushed this down
the stack of things I was involved with (beyond my own work) - KVM
Forum, an acquisition, code freeze, ...

Prior to KVM Forum I asked Peter Krempa (via internal IRC) to look at
patch 3, but he may have forgotten or pushed it down his todo list. Of
course he took an extra week after KVM Forum for holiday - so

Thanks for the nudge, it's on my shorter list of things to do!  I'm
50/50 on the patch 3 event change. I would like to understand why it was
ignored previously. Sometimes there's some very strange and hidden
reason which becomes painfully obvious at some point in the future. Then
it's a search to figure out why the event just popped up, followed by
time spent wondering why we're not filtering the event any more, and why
we filtered it in the first place.

John

> On 17.10.2018 11:41, Nikolay Shirokovskiy wrote:
>>
>>
>> On 16.10.2018 21:40, John Ferlan wrote:
>>>
>>> $SUBJ:
>>>
>>> s/pass/Pass
>>>
>>> On 10/10/18 4:04 AM, Nikolay Shirokovskiy wrote:
>>>> We send duplicate suspended lifecycle events on qemu process stop in several
>>>> places. The reason is stop event handler always send suspended event and
>>>> we addidionally send same event but with more presise reason after call
>>>
>>> *additionally
>>> *precise
>>>
>>>> to qemuProcessStopCPUs. Domain state change is also duplicated.
>>>>
>>>> Let's change domain state and send event only in handler. For this
>>>> purpuse first let's pass state change reason to event handler (event
>>>
>>> *purpose
>>>
>>>> reason is deducible from it).
>>>>
>>>> Inspired by similar patch for resume: 5dab984ed
>>>> "qemu: Pass running reason to RESUME event handler".
>>>>
>>>
>>> In any case, I think the above was a bit more appropriate for the cover
>>> since it details a few things being altered in the 3rd patch of the
>>> series. So, maybe this could change to:
>>>
>>> Similar to commit 5dab984ed which saves and passes the running reason to
>>> the RESUME event handler, during qemuProcessStopCPUs let's save and pass
>>> the pause reason in the domain private data so that the STOP event
>>> handler can use it.
>>>
>>>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
>>>> ---
>>>>  src/qemu/qemu_domain.h  |  4 ++++
>>>>  src/qemu/qemu_process.c | 15 ++++++++++++---
>>>>  2 files changed, 16 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
>>>> index 80bd4bd..380ea14 100644
>>>> --- a/src/qemu/qemu_domain.h
>>>> +++ b/src/qemu/qemu_domain.h
>>>> @@ -370,6 +370,10 @@ struct _qemuDomainObjPrivate {
>>>>      /* qemuProcessStartCPUs stores the reason for starting vCPUs here for the
>>>>       * RESUME event handler to use it */
>>>>      virDomainRunningReason runningReason;
>>>> +
>>>> +    /* qemuProcessStopCPUs stores the reason for starting vCPUs here for the
>>>
>>> s/starting/pausing/
>>>
>>> too much copy-pasta
>>>
>>> FWIW: Similar to the comment I made to Jirka for his series, I assume
>>> this STOP processing would have the similar issue with the event going
>>> to the old libvirtd and thus not needing to save/restore via XML state
>>> processing. For context see:
>>>
>>> https://www.redhat.com/archives/libvir-list/2018-September/msg01145.html
>>>
>>>> +     * STOP event handler to use it */
>>>> +    virDomainPausedReason pausedReason;
>>>>  };
>>>>  
>>>
>>> With a couple of adjustments,
>>>
>>> Reviewed-by: John Ferlan <jferlan at redhat.com>
>>>
>>> John
>>>
>>> I can make the adjustments so that you don't need to send another series
>>> - just need your acknowledgment for that.
>>>
>>
>> I'm ok with you changes
>>
>> Nikolay
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>