[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2] qemu: Report shutdown event details



On Tue, May 16, 2017 at 12:10:23PM -0500, Eric Blake wrote:
> On 05/16/2017 08:49 AM, Martin Kletzander wrote:
> > QEMU will likely report the details of it shutting down, particularly
> > whether the shutdown was initiated by the guest or host.  We should
> > forward that information along, at least for shutdown events.  Reset
> > has that as well, however that is not a lifecycle event and would add
> > extra constants that might not be used.  It can be added later on.
> > 
> > Since the only way we can extend information provided to the user is
> > adding event details, we might as well emit multiple events (one with
> > the reason for the shutdown and keep the one for the shutdown being
> > finished for clarity and compatibility).
> > 
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1384007
> > 
> > Signed-off-by: Martin Kletzander <mkletzan redhat com>
> > ---
> > v2:
> >  - Adapt to new message format
> > 
> > Patch in QEMU:        https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03624.html
> > Applied to qapi-next: https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg03742.html
> 
> Not in qemu master yet, but should land there prior to the next libvirt
> release.
> 
> 
> > +++ b/include/libvirt/libvirt-domain.h
> > @@ -2983,7 +2983,16 @@ typedef enum {
> >   * Details on the cause of a 'shutdown' lifecycle event
> >   */
> >  typedef enum {
> > -    VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0, /* Guest finished shutdown sequence */
> > +    /* Guest finished shutdown sequence */
> > +    VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED = 0,
> > +
> > +    /* Guest is shutting down due to request from guest (e.g. hardware-specific
> > +     * action) */
> > +    VIR_DOMAIN_EVENT_SHUTDOWN_GUEST = 1,
> > +
> > +    /* Guest is shutting down due to request from host (e.g. killed by a
> > +     * signal) */
> > +    VIR_DOMAIN_EVENT_SHUTDOWN_HOST = 2,
> > 
> 
> Looks reasonable.
> 
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -523,9 +523,15 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const char *firstkeyword)
> >  }
> > 
> > 
> > -static void qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, virJSONValuePtr data ATTRIBUTE_UNUSED)
> > +static void qemuMonitorJSONHandleShutdown(qemuMonitorPtr mon, virJSONValuePtr data)
> >  {
> > -    qemuMonitorEmitShutdown(mon);
> > +    bool guest = false;
> > +    virTristateBool guest_initiated = VIR_TRISTATE_BOOL_ABSENT;
> > +
> > +    if (virJSONValueObjectGetBoolean(data, "guest", &guest) == 0)
> > +        guest_initiated = guest ? VIR_TRISTATE_BOOL_YES : VIR_TRISTATE_BOOL_NO;
> > +
> > +    qemuMonitorEmitShutdown(mon, guest_initiated);
> 
> Yes, that uses the new qemu interface correctly.
> 
> > @@ -678,6 +699,7 @@ qemuProcessHandleShutdown(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> > 
> >   unlock:
> >      virObjectUnlock(vm);
> > +    qemuDomainEventQueue(driver, pre_event);
> >      qemuDomainEventQueue(driver, event);
> >      virObjectUnref(cfg);
> 
> Nice - you send the same event as always so old clients don't break, but
> new clients can now look for the new cause.

I don't think that's right actually. IMHO, we should onyl be sending the
new event, not the original event. These are intended to indicate state
changes, and having two VIR_DOMAIN_EVENT_SHUTDOWN events sent with
different details is not really representing a state change.

WRT to compatibility, clients should always expect that 'detail' may
change or new 'detail' enum values may be added - indeed we've done
that many many times int he past. So I don't think we need to duplicate
the existing event


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]