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 :|