[libvirt] [PATCH] [DRAFT] handle qemu event GUEST_PANICKED

This is for early review, comments are welcome! qemu is adding a method to detect guest panic, and a new event GUEST_PANICKED[1]. This patch adds support for GUEST_PANICKED. But I have two questions: - to react to GUEST_PANICKED, can xml element <on_crash> be used as the action to take, or do I have to invent another xml element to configure the action? - Currently libvirt relies on watchdog to detect guest panic, but this way is not that reliable(there has to be a watchdog device, the watchdog itself has to be working when panic occurs), so it seems to be appropriate to remove this method from libvirt as there is a better one. But, will the removal cause any problems, such as backward compatibility, user confusion, etc.? [1] http://lists.nongnu.org/archive/html/qemu-devel/2012-10/msg04361.html --- src/qemu/qemu_command.c | 31 ++++++++++++++++++++----------- src/qemu/qemu_monitor.c | 10 ++++++++++ src/qemu/qemu_monitor.h | 3 +++ src/qemu/qemu_monitor_json.c | 9 +++++++++ src/qemu/qemu_process.c | 9 +++++++++ 5 files changed, 51 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 898c4c0..0c3f9ff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4300,6 +4300,9 @@ qemuBuildMachineArgStr(virCommandPtr cmd, const virDomainDefPtr def, qemuCapsPtr caps) { + virBuffer params = VIR_BUFFER_INITIALIZER; + char *tmp = NULL; + /* This should *never* be NULL, since we always provide * a machine in the capabilities data for QEMU. So this * check is just here as a safety in case the unexpected @@ -4307,29 +4310,35 @@ qemuBuildMachineArgStr(virCommandPtr cmd, if (!def->os.machine) return 0; - if (!def->mem.dump_core) { - /* if no parameter to the machine type is needed, we still use - * '-M' to keep the most of the compatibility with older versions. - */ - virCommandAddArgList(cmd, "-M", def->os.machine, NULL); - } else { + virBufferAsprintf(¶ms, ",enable_pv_event=on"); + + if (def->mem.dump_core) { if (!qemuCapsGet(caps, QEMU_CAPS_DUMP_GUEST_CORE)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("dump-guest-core is not available " " with this QEMU binary")); return -1; } + virBufferAsprintf(¶ms,",%s", + virDomainMemDumpTypeToString(def->mem.dump_core)); + } - /* However, in case there is a parameter to be added, we need to + tmp = virBufferContentAndReset(¶ms); + if (!tmp || strlen(tmp) == 0) { + /* if no parameter to the machine type is needed, we still use + * '-M' to keep the most of the compatibility with older versions. + */ + virCommandAddArgList(cmd, "-M", def->os.machine, NULL); + } else { + /* However, in case there are parameters to be added, we need to * use the "-machine" parameter because qemu is not parsing the * "-M" correctly */ virCommandAddArg(cmd, "-machine"); - virCommandAddArgFormat(cmd, - "%s,dump-guest-core=%s", - def->os.machine, - virDomainMemDumpTypeToString(def->mem.dump_core)); + virCommandAddArgFormat(cmd, "%s%s", def->os.machine, tmp); } + VIR_FREE(tmp); + return 0; } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 2d9c44c..8ab7ed2 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1146,6 +1146,16 @@ int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon, } +int qemuMonitorEmitPanic(qemuMonitorPtr mon) +{ + int ret = -1; + VIR_DEBUG("mon=%p", mon); + + QEMU_MONITOR_CALLBACK(mon, ret, domainPanic, mon->vm); + return ret; +} + + int qemuMonitorSetCapabilities(qemuMonitorPtr mon) { int ret; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 8856d9f..ea0a474 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -138,6 +138,8 @@ struct _qemuMonitorCallbacks { unsigned long long actual); int (*domainPMSuspendDisk)(qemuMonitorPtr mon, virDomainObjPtr vm); + int (*domainPanic)(qemuMonitorPtr mon, + virDomainObjPtr vm); }; char *qemuMonitorEscapeArg(const char *in); @@ -216,6 +218,7 @@ int qemuMonitorEmitBlockJob(qemuMonitorPtr mon, int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon, unsigned long long actual); int qemuMonitorEmitPMSuspendDisk(qemuMonitorPtr mon); +int qemuMonitorEmitPanic(qemuMonitorPtr mon); int qemuMonitorStartCPUs(qemuMonitorPtr mon, virConnectPtr conn); diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index e495c0a..0adbefc 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -71,6 +71,8 @@ static void qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon, virJSONVa static void qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandleBalloonChange(qemuMonitorPtr mon, virJSONValuePtr data); static void qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, virJSONValuePtr data); +static void qemuMonitorJSONHandlePanic(qemuMonitorPtr mon, virJSONValuePtr data); + typedef struct { const char *type; @@ -83,6 +85,7 @@ static qemuEventHandler eventHandlers[] = { { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, }, { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, }, { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, }, + { "GUEST_PANICKED", qemuMonitorJSONHandlePanic, }, { "POWERDOWN", qemuMonitorJSONHandlePowerdown, }, { "RESET", qemuMonitorJSONHandleReset, }, { "RTC_CHANGE", qemuMonitorJSONHandleRTCChange, }, @@ -4417,3 +4420,9 @@ cleanup: virJSONValueFree(reply); return ret; } + +static void qemuMonitorJSONHandlePanic(qemuMonitorPtr mon, + virJSONValuePtr data ATTRIBUTE_UNUSED) +{ + qemuMonitorEmitPanic(mon); +} diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 969e3ce..d3b37e0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1235,6 +1235,14 @@ qemuProcessHandlePMSuspendDisk(qemuMonitorPtr mon ATTRIBUTE_UNUSED, return 0; } +static int +qemuProcessHandlePanic(qemuMonitorPtr mon ATTRIBUTE_UNUSED, + virDomainObjPtr vm ATTRIBUTE_UNUSED) +{ + VIR_WARN("guest panicked."); + return 0; +} + static qemuMonitorCallbacks monitorCallbacks = { .destroy = qemuProcessHandleMonitorDestroy, @@ -1254,6 +1262,7 @@ static qemuMonitorCallbacks monitorCallbacks = { .domainPMSuspend = qemuProcessHandlePMSuspend, .domainBalloonChange = qemuProcessHandleBalloonChange, .domainPMSuspendDisk = qemuProcessHandlePMSuspendDisk, + .domainPanic = qemuProcessHandlePanic, }; static int -- 1.7.10.2

On Thu, Oct 25, 2012 at 04:04:23PM +0800, Hu Tao wrote:
This is for early review, comments are welcome!
qemu is adding a method to detect guest panic, and a new event GUEST_PANICKED[1]. This patch adds support for GUEST_PANICKED.
But I have two questions:
- to react to GUEST_PANICKED, can xml element <on_crash> be used as the action to take, or do I have to invent another xml element to configure the action?
Using <on_crash> is the right thing todo, since that matches what we do with Xen.
- Currently libvirt relies on watchdog to detect guest panic, but this way is not that reliable(there has to be a watchdog device, the watchdog itself has to be working when panic occurs), so it seems to be appropriate to remove this method from libvirt as there is a better one. But, will the removal cause any problems, such as backward compatibility, user confusion, etc.?
I don't think we need to touch the watchdog code at all. Not all guest OS will support this new GUEST_PANICKED event, while they probably do have drivers for the watchdog devices. So it is perfectly ok to have both the panick event and the watchdog events available. Mgmt apps can decide which to use as they see fit. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange
-
Hu Tao