[libvirt] [PATCH v5 2/5] qemu: Supports guest panicked

[Eric Blake]

On 06/05/2013 03:54 AM, Chen Fan wrote:
> Add monitor callback API domainGUESTPanicked, that
> implements the 'on_crash' behavior in the XML when domain crashed.
> ---
>  src/qemu/qemu_monitor.c      |  14 +++++-
>  src/qemu/qemu_monitor.h      |   5 +++
>  src/qemu/qemu_monitor_json.c |   7 +++
>  src/qemu/qemu_process.c      | 103 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 127 insertions(+), 2 deletions(-)
> 
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 4e35f79..e0cd62c 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -113,7 +113,7 @@ VIR_ENUM_IMPL(qemuMonitorVMStatus,
>                QEMU_MONITOR_VM_STATUS_LAST,
>                "debug", "inmigrate", "internal-error", "io-error", "paused",
>                "postmigrate", "prelaunch", "finish-migrate", "restore-vm",
> -              "running", "save-vm", "shutdown", "watchdog")
> +              "running", "save-vm", "shutdown", "watchdog", "guest-panicked")

If this is internal-only, can we use the shorter name 'guest-panic'?

>  
>  typedef enum {
>      QEMU_MONITOR_BLOCK_IO_STATUS_OK,
> @@ -1032,6 +1032,15 @@ int qemuMonitorEmitResume(qemuMonitorPtr mon)
>  }
>  
>  
> +int qemuMonitorEmitGUESTPanicked(qemuMonitorPtr mon)

WHY THE ALL CAPS?  qemuMonitorEmitGuestPanic() is sufficient for naming
purposes.

> +{
> +    int ret = -1;
> +    VIR_DEBUG("mon=%p", mon);
> +    QEMU_MONITOR_CALLBACK(mon, ret, domainGUESTPanicked, mon->vm);
> +    return ret;
> +}
> +
> +
>  int qemuMonitorEmitRTCChange(qemuMonitorPtr mon, long long offset)

If it was because of copy-and-paste, remember that RTC is an acronym,
but Guest is not.

> +++ b/src/qemu/qemu_monitor.h
> @@ -140,6 +140,8 @@ struct _qemuMonitorCallbacks {
>                                 unsigned long long actual);
>      int (*domainPMSuspendDisk)(qemuMonitorPtr mon,
>                                 virDomainObjPtr vm);
> +    int (*domainGUESTPanicked)(qemuMonitorPtr mon,

Again, s/GUEST/Guest/

> +                               virDomainObjPtr vm);
>  };
>  
>  char *qemuMonitorEscapeArg(const char *in);
> @@ -220,6 +222,8 @@ int qemuMonitorEmitBlockJob(qemuMonitorPtr mon,
>  int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon,
>                                   unsigned long long actual);
>  int qemuMonitorEmitPMSuspendDisk(qemuMonitorPtr mon);
> +int qemuMonitorEmitGUESTPanicked(qemuMonitorPtr mon);

and again.


> +++ b/src/qemu/qemu_monitor_json.c
> @@ -74,6 +74,7 @@ static void qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon, virJSONVal
>  static void qemuMonitorJSONHandleBlockJobReady(qemuMonitorPtr mon, virJSONValuePtr data);
>  static void qemuMonitorJSONHandleBalloonChange(qemuMonitorPtr mon, virJSONValuePtr data);
>  static void qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, virJSONValuePtr data);
> +static void qemuMonitorJSONHandleGUESTPanicked(qemuMonitorPtr mon, virJSONValuePtr data);

and again

> +static int
> +qemuProcessHandleGUESTPanicked(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
> +                               virDomainObjPtr vm)
> +{

> +
> +    if (vm->def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_PRESERVE) {
...
> +        goto cleanup;
> +    }
> +
> +    if (vm->def->onCrash == VIR_DOMAIN_LIFECYCLE_CRASH_DESTROY) {

Worth doing this as 'else if'?

> +        isReboot = false;
> +        VIR_INFO("Domain on_crash setting overridden, shutting down");

That sounds like fake reboot is overriding on_crash settings.  I'd word
this:

on_crash overrides fake reboot request, shutting down instead

and maybe be careful to only output that info message only if isReboot
actually changed from true to false.

> +
> +        if (! virDomainObjIsActive(vm)) {

Generally no space after '!'.

> @@ -2673,6 +2770,10 @@ qemuProcessUpdateState(virQEMUDriverPtr driver, virDomainObjPtr vm)
>              newState = VIR_DOMAIN_SHUTDOWN;
>              newReason = VIR_DOMAIN_SHUTDOWN_UNKNOWN;
>              ignore_value(VIR_STRDUP_QUIET(msg, "shutdown"));
> +        } else if (reason == VIR_DOMAIN_PAUSED_GUEST_PANICKED) {
> +            newState = VIR_DOMAIN_CRASHED;
> +            newReason = VIR_DOMAIN_CRASHED_PANICKED;
> +            ignore_value(VIR_STRDUP_QUIET(msg, "was crashed"));

s/was //

Overall, the mechanics seem like they will work, but the spelling to use
Guest instead of GUEST in function names probably warrants a respin.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130605/4b0ba2a6/attachment-0001.sig>