Re: [libvirt] [PATCH v2] qemu: Report shutdown event details
On Tue, May 16, 2017 at 01:51:17PM -0500, Eric Blake wrote: On 05/16/2017 12:20 PM, Daniel P. Berrange wrote: @@ -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 That may be my fault for causing a mis-understanding of back-compatibility handling on my review of v1. In the past, when we've had an event that returns a too-small struct, the only way to return more information is to create a second event with the additional info, then fire off both events at the same time (for the clients expecting the old event semantics, and for new clients) - which really means two separate RPC events. But here, we already have sufficient lifecycle event parameters to return details without having to add a new RPC event (proven by the fact that you didn't have to touch src/remote at all). So now I'm agreeing with Daniel - the fact that we have new information means we don't need to be back-compat to older clients: they will see the same lifecycle event they have always seen, and not care that the cause has changed, while new clients will see a plain cause where no information was available from older qemu, or one of the two new causes where qemu gave it to us. Sure, I'll give that a respin ;) Thanks for the review. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Report shutdown event details
On 05/16/2017 12:20 PM, Daniel P. Berrange wrote: >>> @@ -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 That may be my fault for causing a mis-understanding of back-compatibility handling on my review of v1. In the past, when we've had an event that returns a too-small struct, the only way to return more information is to create a second event with the additional info, then fire off both events at the same time (for the clients expecting the old event semantics, and for new clients) - which really means two separate RPC events. But here, we already have sufficient lifecycle event parameters to return details without having to add a new RPC event (proven by the fact that you didn't have to touch src/remote at all). So now I'm agreeing with Daniel - the fact that we have new information means we don't need to be back-compat to older clients: they will see the same lifecycle event they have always seen, and not care that the cause has changed, while new clients will see a plain cause where no information was available from older qemu, or one of the two new causes where qemu gave it to us. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] qemu: Report shutdown event details
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
> ---
> 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.
ACK
although I'd wait until it is actually in qemu.git master before pushing
to libvirt.git
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
[libvirt] [PATCH v2] qemu: Report shutdown event details
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
---
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
examples/object-events/event-test.c | 6 ++
include/libvirt/libvirt-domain.h| 11 ++-
src/qemu/qemu_monitor.c | 6 +++---
src/qemu/qemu_monitor.h | 3 ++-
src/qemu/qemu_monitor_json.c| 10 --
src/qemu/qemu_process.c | 22 ++
tools/virsh-domain.c| 5 -
7 files changed, 55 insertions(+), 8 deletions(-)
diff --git a/examples/object-events/event-test.c
b/examples/object-events/event-test.c
index 12690cac09ce..d3a81eb4a559 100644
--- a/examples/object-events/event-test.c
+++ b/examples/object-events/event-test.c
@@ -240,6 +240,12 @@ eventDetailToString(int event,
case VIR_DOMAIN_EVENT_SHUTDOWN_FINISHED:
return "Finished";
+case VIR_DOMAIN_EVENT_SHUTDOWN_GUEST:
+return "Killed on a guest request";
+
+case VIR_DOMAIN_EVENT_SHUTDOWN_HOST:
+return "Killed on a host request";
+
case VIR_DOMAIN_EVENT_SHUTDOWN_LAST:
break;
}
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index c9e96a6c90bc..de391a9214d1 100644
--- a/include/libvirt/libvirt-domain.h
+++ 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,
# ifdef VIR_ENUM_SENTINELS
VIR_DOMAIN_EVENT_SHUTDOWN_LAST
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 2148d483ed6a..a2de1a6c5bad 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1326,13 +1326,13 @@ qemuMonitorEmitEvent(qemuMonitorPtr mon, const char
*event,
int
-qemuMonitorEmitShutdown(qemuMonitorPtr mon)
+qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest)
{
int ret = -1;
-VIR_DEBUG("mon=%p", mon);
+VIR_DEBUG("mon=%p guest=%u", mon, guest);
mon->willhangup = 1;
-QEMU_MONITOR_CALLBACK(mon, ret, domainShutdown, mon->vm);
+QEMU_MONITOR_CALLBACK(mon, ret, domainShutdown, mon->vm, guest);
return ret;
}
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 12f98beba763..8956bf929aaa 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -130,6 +130,7 @@ typedef int
(*qemuMonitorDomainEventCallback)(qemuMonitorPtr mon,
void *opaque);
typedef int (*qemuMonitorDomainShutdownCallback)(qemuMonitorPtr mon,
virDomainObjPtr vm,
+ virTristateBool guest,
void *opaque);
typedef int (*qemuMonitorDomainResetCallback)(qemuMonitorPtr mon,
virDomainObjPtr vm,
@@ -344,7 +345,7 @@ int qemuMonitorGetDiskSecret(qemuMonitorPtr mon,
int qemuMonitorEmitEvent(qemuMonitorPtr mon, const char *event,
long long seconds, unsigned int micros,
const char *details);
-int qemuMonitorEmitShutdown(qemuMonitorPtr mon);
+int qemuMonitorEmitShutdown(qemuMonitorPtr mon, virTristateBool guest);
int qemuMonitorEmitReset(qemuMonitorPtr mon);
int qemuMonitorEmitPowerdown(qemuMonitorPtr mon);
int qemuMonitorEmitStop(qemuMonitorPtr mon);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 083729003ba3..757595dd7472 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -523,9 +523,15 @@ qemuMonitorJSONKeywordStringToJSON(const char *str, const
char *firstkeyword)
}
-stat
