[libvirt] [PATCH 0/9] extend virsh domstate to show additional information
This patch series introduces the ability to save additional information for the domain state and exposes this information in virsh domstate. For example in the case of QEMU guest panic events, we can provide additional information like the crash reason or register state of the domain. This information usually gets logged in the domain log but for debugging it is useful to have it accessible from the client. Therefore, let's introduce a new public API function, virDomainGetStateParams, an extensible version of virDomainGetState, which returns the complete state of the domain, including newly introduced additional information. Let's also extend virsh domstate and introduce a new parameter --info to show the domain state, reason and additional information when available. virsh # domstate --info guest-1 crashed (panicked: disabled-wait core='1' psw-mask='0x000000000010f146' \ psw-addr='0x0002000180000000') Bjoern Walk (9): conf: add info to virDomainStateReason conf: set/retrieve state information lib: introduce virDomainGetStateParams function remote: implement remoteDomainGetStateParams qemu: implement qemuDomainGetStateParams qemu: set state information for guest panic event virsh: domstate: report detailed state if available news: add entry for virDomainGetStateParams qemu: fix order of S390 panic event information docs/news.xml | 11 ++++++ include/libvirt/libvirt-domain.h | 24 ++++++++++++ src/conf/domain_conf.c | 16 +++++++- src/conf/domain_conf.h | 7 ++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++ src/libvirt_private.syms | 1 + src/libvirt_public.syms | 5 +++ src/qemu/qemu_driver.c | 61 ++++++++++++++++++++++++++++- src/qemu/qemu_monitor.c | 39 ++++++++++++++++-- src/qemu/qemu_monitor.h | 1 + src/remote/remote_daemon_dispatch.c | 1 - src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 20 +++++++++- src/remote_protocol-structs | 11 ++++++ tools/virsh-domain-monitor.c | 31 ++++++++++++--- tools/virsh.pod | 5 ++- 17 files changed, 281 insertions(+), 16 deletions(-) -- 2.17.0
On some architectures, QEMU exposes additional information on certain domain states, e.g. for guest crashes. Let's add a field @info to the virDomainStateReason struct inside virDomainObj which holds additional state information. One drawback is that this information is not serialized and is lost in case of a daemon restart. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/conf/domain_conf.c | 1 + src/conf/domain_conf.h | 1 + 2 files changed, 2 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7396616e..805da60d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -3182,6 +3182,7 @@ static void virDomainObjDispose(void *obj) VIR_DEBUG("obj=%p", dom); virCondDestroy(&dom->cond); + VIR_FREE(dom->state.info); virDomainDefFree(dom->def); virDomainDefFree(dom->newDef); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 0f10e242..d4fd676f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2578,6 +2578,7 @@ typedef struct _virDomainStateReason virDomainStateReason; struct _virDomainStateReason { int state; int reason; + char *info; }; typedef struct _virDomainObj virDomainObj; -- 2.17.0
Add the ability to set and retrieve additional state information for the domain. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/conf/domain_conf.c | 15 ++++++++++++++- src/conf/domain_conf.h | 6 ++++++ src/libvirt_private.syms | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 805da60d..211a77a2 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -28766,7 +28766,10 @@ virDomainObjGetState(virDomainObjPtr dom, int *reason) void -virDomainObjSetState(virDomainObjPtr dom, virDomainState state, int reason) +virDomainObjSetStateFull(virDomainObjPtr dom, + virDomainState state, + int reason, + const char *info) { int last; @@ -28806,6 +28809,16 @@ virDomainObjSetState(virDomainObjPtr dom, virDomainState state, int reason) dom->state.reason = reason; else dom->state.reason = 0; + + VIR_FREE(dom->state.info); + ignore_value(VIR_STRDUP(dom->state.info, info)); +} + + +void +virDomainObjSetState(virDomainObjPtr dom, virDomainState state, int reason) +{ + virDomainObjSetStateFull(dom, state, reason, NULL); } diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index d4fd676f..b4be3c49 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3320,6 +3320,12 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, void *opaque); void +virDomainObjSetStateFull(virDomainObjPtr obj, + virDomainState state, + int reason, + const char *info) + ATTRIBUTE_NONNULL(1); +void virDomainObjSetState(virDomainObjPtr obj, virDomainState state, int reason) ATTRIBUTE_NONNULL(1); virDomainState diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e688981c..a1b28ccd 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -484,6 +484,7 @@ virDomainObjRemoveTransientDef; virDomainObjSetDefTransient; virDomainObjSetMetadata; virDomainObjSetState; +virDomainObjSetStateFull; virDomainObjTaint; virDomainObjUpdateModificationImpact; virDomainObjWait; -- 2.17.0
This API function extends and generalizes the virDomainGetState function by returning a dictionary of typed parameters. In later commits, the domain state, reason and the previously introduced additional state information are returned. Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- include/libvirt/libvirt-domain.h | 24 ++++++++++++++ src/driver-hypervisor.h | 7 ++++ src/libvirt-domain.c | 56 ++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 5 +++ 4 files changed, 92 insertions(+) diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h index 796f2e14..c7b85cd0 100644 --- a/include/libvirt/libvirt-domain.h +++ b/include/libvirt/libvirt-domain.h @@ -1254,6 +1254,30 @@ int virDomainGetState (virDomainPtr domain, int *reason, unsigned int flags); +/** + * VIR_DOMAIN_STATE_PARAMS_STATE: + * state of the domain (cf. virDomainState) as an int + */ +# define VIR_DOMAIN_STATE_PARAMS_STATE "state" + +/** + * VIR_DOMAIN_STATE_PARAMS_REASON: + * state reason of the domain (one of virDomain*Reason) as an int + */ +# define VIR_DOMAIN_STATE_PARAMS_REASON "reason" + +/** + * VIR_DOMAIN_STATE_PARAMS_INFO: + * additional information for certain state reasons as a string + */ +# define VIR_DOMAIN_STATE_PARAMS_INFO "info" + +int virDomainGetStateParams(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags); + + /** * VIR_DOMAIN_CPU_STATS_CPUTIME: * cpu usage (sum of both vcpu and hypervisor usage) in nanoseconds, diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index eef31eb1..2b3fe725 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -220,6 +220,12 @@ typedef int int *reason, unsigned int flags); +typedef int +(*virDrvDomainGetStateParams)(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags); + typedef int (*virDrvDomainGetControlInfo)(virDomainPtr domain, virDomainControlInfoPtr info, @@ -1383,6 +1389,7 @@ struct _virHypervisorDriver { virDrvDomainGetBlkioParameters domainGetBlkioParameters; virDrvDomainGetInfo domainGetInfo; virDrvDomainGetState domainGetState; + virDrvDomainGetStateParams domainGetStateParams; virDrvDomainGetControlInfo domainGetControlInfo; virDrvDomainSave domainSave; virDrvDomainSaveFlags domainSaveFlags; diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index ab7266dc..78d95bf6 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -2502,6 +2502,62 @@ virDomainGetState(virDomainPtr domain, } +/** + * virDomainGetStateParams: + * @domain: a domain object + * @params: where to store domain statistics, must be freed by the caller + * @nparams: number of items in @params + * @flags: bitwise-OR of virDomainGetStateFlags, + * not currently used yet, callers should always pass 0 + * + * Extract domain state. Each state is accompanied by a reason (if known) + * and optional detailed information. + * + * Possible fields returned in @params are defined by VIR_DOMAIN_STATE_PARAMS_* + * macros and new fields will likely be introduced in the future so callers + * may receive fields that they do not understand in case they talk to a newer + * server. The caller is responsible to free allocated memory returned in + * @params by calling virTypedParamsFree. + * + * Returns 0 in case of success and -1 in case of failure. + */ +int +virDomainGetStateParams(virDomainPtr domain, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%p, flags=0x%x", + params, nparams, flags); + + virResetLastError(); + + virCheckDomainReturn(domain, -1); + virCheckNonNullArgGoto(params, error); + virCheckNonNullArgGoto(nparams, error); + + conn = domain->conn; + if (conn->driver->domainGetStateParams) { + int ret; + ret = conn->driver->domainGetStateParams(domain, + params, + nparams, + flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(domain->conn); + return -1; +} + + /** * virDomainGetControlInfo: * @domain: a domain object diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index d4cdbd8b..866c2e99 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -809,4 +809,9 @@ LIBVIRT_4.5.0 { virNWFilterBindingGetFilterName; } LIBVIRT_4.4.0; +LIBVIRT_4.6.0 { + global: + virDomainGetStateParams; +} LIBVIRT_4.5.0; + # .... define new API here using predicted next version number .... -- 2.17.0
Implement the API function virDomainGetStateParams for the remote driver and wire up the remote protocol. Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/remote/remote_daemon_dispatch.c | 1 - src/remote/remote_driver.c | 1 + src/remote/remote_protocol.x | 20 +++++++++++++++++++- src/remote_protocol-structs | 11 +++++++++++ 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index 4a93f09a..57623b6d 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -4159,7 +4159,6 @@ remoteDispatchDomainGetState(virNetServerPtr server ATTRIBUTE_UNUSED, return rv; } - /* Due to back-compat reasons, two RPC calls map to the same libvirt * API of virConnectDomainEventRegisterAny. A client should only use * the new call if they have probed diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 1d94c2e4..e2a79c48 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -8347,6 +8347,7 @@ static virHypervisorDriver hypervisor_driver = { .domainSetPerfEvents = remoteDomainSetPerfEvents, /* 1.3.3 */ .domainGetInfo = remoteDomainGetInfo, /* 0.3.0 */ .domainGetState = remoteDomainGetState, /* 0.9.2 */ + .domainGetStateParams = remoteDomainGetStateParams, /* 4.6.0 */ .domainGetControlInfo = remoteDomainGetControlInfo, /* 0.9.3 */ .domainSave = remoteDomainSave, /* 0.3.0 */ .domainSaveFlags = remoteDomainSaveFlags, /* 0.9.4 */ diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 28c8feba..43d5db89 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -262,6 +262,9 @@ const REMOTE_NODE_SEV_INFO_MAX = 64; /* Upper limit on number of launch security information entries */ const REMOTE_DOMAIN_LAUNCH_SECURITY_INFO_PARAMS_MAX = 64; +/* Upper limit on number of state parameter entries */ +const REMOTE_DOMAIN_STATE_PARAMETERS_MAX = 16; + /* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */ typedef opaque remote_uuid[VIR_UUID_BUFLEN]; @@ -2781,6 +2784,15 @@ struct remote_domain_get_state_ret { int reason; }; +struct remote_domain_get_state_params_args { + remote_nonnull_domain dom; + unsigned int flags; +}; + +struct remote_domain_get_state_params_ret { + remote_typed_param params<REMOTE_DOMAIN_STATE_PARAMETERS_MAX>; /* alloc@1@int@2 */ +}; + struct remote_domain_migrate_begin3_args { remote_nonnull_domain dom; remote_string xmlin; @@ -6312,5 +6324,11 @@ enum remote_procedure { * @acl: connect:search_nwfilter_bindings * @aclfilter: nwfilter_binding:getattr */ - REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401 + REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401, + + /** + * @generate: both + * @acl: domain:read + */ + REMOTE_PROC_DOMAIN_GET_STATE_PARAMS = 402 }; diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index 6343e146..a19b9730 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -2153,6 +2153,16 @@ struct remote_domain_get_state_ret { int state; int reason; }; +struct remote_domain_get_state_params_args { + remote_nonnull_domain dom; + u_int flags; +}; +struct remote_domain_get_state_params_ret { + struct { + u_int params_len; + remote_typed_param * params_val; + } params; +}; struct remote_domain_migrate_begin3_args { remote_nonnull_domain dom; remote_string xmlin; @@ -3368,4 +3378,5 @@ enum remote_procedure { REMOTE_PROC_NWFILTER_BINDING_CREATE_XML = 399, REMOTE_PROC_NWFILTER_BINDING_DELETE = 400, REMOTE_PROC_CONNECT_LIST_ALL_NWFILTER_BINDINGS = 401, + REMOTE_PROC_DOMAIN_GET_STATE_PARAMS = 402, }; -- 2.17.0
Implement the API function virDomainGetStateParams for the QEMU hypervisor driver. Reviewed-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/qemu/qemu_driver.c | 50 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 5de9aaef..871f2dde 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2671,6 +2671,55 @@ qemuDomainGetState(virDomainPtr dom, return ret; } +static int +qemuDomainGetStateParams(virDomainPtr dom, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + int ret = -1; + virDomainObjPtr vm; + virTypedParameterPtr par = NULL; + int npar, maxparams = 0; + + virCheckFlags(0, -1); + + if (!(vm = qemuDomObjFromDomain(dom))) + goto cleanup; + + if (virDomainGetStateParamsEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virTypedParamsAddInt(&par, &npar, + &maxparams, + VIR_DOMAIN_STATE_PARAMS_STATE, + vm->state.state) < 0) { + goto cleanup; + } + if (virTypedParamsAddInt(&par, &npar, + &maxparams, + VIR_DOMAIN_STATE_PARAMS_REASON, + vm->state.reason) < 0) { + goto cleanup; + } + if (virTypedParamsAddString(&par, &npar, + &maxparams, + VIR_DOMAIN_STATE_PARAMS_INFO, + vm->state.info) < 0) { + goto cleanup; + } + + VIR_STEAL_PTR(*params, par); + *nparams = npar; + npar = 0; + ret = 0; + + cleanup: + virTypedParamsFree(par, npar); + virDomainObjEndAPI(&vm); + return ret; +} + static int qemuDomainGetControlInfo(virDomainPtr dom, virDomainControlInfoPtr info, @@ -21664,6 +21713,7 @@ static virHypervisorDriver qemuHypervisorDriver = { .domainGetBlkioParameters = qemuDomainGetBlkioParameters, /* 0.9.0 */ .domainGetInfo = qemuDomainGetInfo, /* 0.2.0 */ .domainGetState = qemuDomainGetState, /* 0.9.2 */ + .domainGetStateParams = qemuDomainGetStateParams, /* 4.6.0 */ .domainGetControlInfo = qemuDomainGetControlInfo, /* 0.9.3 */ .domainSave = qemuDomainSave, /* 0.2.0 */ .domainSaveFlags = qemuDomainSaveFlags, /* 0.9.4 */ -- 2.17.0
Since version 2.12, QEMU reports additional information in case of a guest panic. Let's set the state information when handling the guest panic event and make it available for later. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/qemu/qemu_driver.c | 11 +++++++++-- src/qemu/qemu_monitor.c | 31 +++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.h | 1 + 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 871f2dde..495c38db 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4255,6 +4255,7 @@ processGuestPanicEvent(virQEMUDriverPtr driver, qemuDomainObjPrivatePtr priv = vm->privateData; virObjectEventPtr event = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); + char *state_info = NULL; bool removeInactive = false; unsigned long flags = VIR_DUMP_MEMORY_ONLY; @@ -4268,10 +4269,15 @@ processGuestPanicEvent(virQEMUDriverPtr driver, goto endjob; } - if (info) + if (info) { qemuProcessGuestPanicEventInfo(driver, vm, info); + state_info = qemuMonitorGuestPanicEventInfoFormatMsgCompact(info); + } - virDomainObjSetState(vm, VIR_DOMAIN_CRASHED, VIR_DOMAIN_CRASHED_PANICKED); + virDomainObjSetStateFull(vm, + VIR_DOMAIN_CRASHED, + VIR_DOMAIN_CRASHED_PANICKED, + state_info); event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_CRASHED, @@ -4329,6 +4335,7 @@ processGuestPanicEvent(virQEMUDriverPtr driver, qemuDomainRemoveInactiveJob(driver, vm); cleanup: + VIR_FREE(state_info); virObjectUnref(cfg); } diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index bc116e4e..1ca54283 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4240,6 +4240,37 @@ qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon) } +char * +qemuMonitorGuestPanicEventInfoFormatMsgCompact(qemuMonitorEventPanicInfoPtr info) +{ + char *ret = NULL; + + switch (info->type) { + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_HYPERV: + ignore_value(virAsprintf(&ret, + "arg1='0x%llx', arg2='0x%llx', " + "arg3='0x%llx', arg4='0x%llx', arg5='0x%llx'", + info->data.hyperv.arg1, info->data.hyperv.arg2, + info->data.hyperv.arg3, info->data.hyperv.arg4, + info->data.hyperv.arg5)); + break; + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390: + ignore_value(virAsprintf(&ret, "%s core='%d' psw-mask='0x%016llx' " + "psw-addr='0x%016llx'", + info->data.s390.reason, + info->data.s390.core, + info->data.s390.psw_mask, + info->data.s390.psw_addr)); + break; + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE: + case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST: + break; + } + + return ret; +} + + char * qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index e8ed2d04..e6fb39c5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -109,6 +109,7 @@ struct _qemuMonitorEventPanicInfo { } data; }; +char *qemuMonitorGuestPanicEventInfoFormatMsgCompact(qemuMonitorEventPanicInfoPtr info); char *qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info); void qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info); -- 2.17.0
Add a new parameter to virsh domstate, --info, to additionally report additional information for the domain state: virsh # domstate --info guest-1 crashed (panicked: disabled-wait core='1' psw-mask='0x000000000010f146' \ psw-addr='0x0002000180000000') When the new API virDomainGetStateParams is not available for the server or not supported by the hypervisor driver, fall back to the old API using virDomainGetState function. The --info parameter implies the --reason parameter and if additional information is not available, the output is the same. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Stefan Zimmermann <stzi@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- tools/virsh-domain-monitor.c | 31 ++++++++++++++++++++++++++----- tools/virsh.pod | 5 +++-- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 87660ee6..ed732126 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -1372,6 +1372,10 @@ static const vshCmdOptDef opts_domstate[] = { .type = VSH_OT_BOOL, .help = N_("also print reason for the state") }, + {.name = "info", + .type = VSH_OT_BOOL, + .help = N_("also print reason and information for the state") + }, {.name = NULL} }; @@ -1381,26 +1385,43 @@ cmdDomstate(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom; bool ret = true; bool showReason = vshCommandOptBool(cmd, "reason"); + bool showInfo = vshCommandOptBool(cmd, "info"); + virTypedParameterPtr params = NULL; + int nparams = 0; int state, reason; + const char *info = NULL; if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) return false; - if ((state = virshDomainState(ctl, dom, &reason)) < 0) { - ret = false; - goto cleanup; + if (virDomainGetStateParams(dom, ¶ms, &nparams, 0) < 0 || + virTypedParamsGetInt(params, nparams, VIR_DOMAIN_STATE_PARAMS_STATE, + &state) < 0 || + virTypedParamsGetInt(params, nparams, VIR_DOMAIN_STATE_PARAMS_REASON, + &reason) < 0 || + virTypedParamsGetString(params, nparams, VIR_DOMAIN_STATE_PARAMS_INFO, + &info) < 0) { + if ((state = virshDomainState(ctl, dom, &reason)) < 0) { + ret = false; + goto cleanup; + } } - if (showReason) { - vshPrint(ctl, "%s (%s)\n", + if (showInfo || showReason) { + vshPrint(ctl, "%s (%s", virshDomainStateToString(state), virshDomainStateReasonToString(state, reason)); + if (showInfo && info && strlen(info) > 0) + vshPrint(ctl, ": %s)\n", info); + else + vshPrint(ctl, ")\n"); } else { vshPrint(ctl, "%s\n", virshDomainStateToString(state)); } cleanup: + virTypedParamsFree(params, nparams); virshDomainFree(dom); return ret; } diff --git a/tools/virsh.pod b/tools/virsh.pod index 4c6e21fc..b30b0b79 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -1519,10 +1519,11 @@ specified in the second argument. B<Note>: Domain must be inactive and without snapshots. -=item B<domstate> I<domain> [I<--reason>] +=item B<domstate> I<domain> [I<--reason>] [I<--info>] Returns state about a domain. I<--reason> tells virsh to also print -reason for the state. +reason for the state. I<--info> prints additional state information if +available, I<--info> implies I<--reason>. =item B<domcontrol> I<domain> -- 2.17.0
Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 773c95b0..6cbd2c78 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -35,6 +35,17 @@ <libvirt> <release version="v4.6.0" date="unreleased"> <section title="New features"> + <change> + <summary> + Ability to set/get additional state information + </summary> + <description> + Add the ability to set/get additional state information, e.g. guest + CPU state for crashed guests. Introduce a new extensible API + function, virDomainGetStateParams, and add new argument --info to + the virsh domstate command. + </description> + </change> <change> <summary> qemu: Implement the HTM pSeries feature -- 2.17.0
Let's make the formatting of the log output consistent to the domstate output and put the reason first. Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/qemu/qemu_monitor.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 1ca54283..47af8a8d 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -4286,12 +4286,12 @@ qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info) info->data.hyperv.arg5)); break; case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390: - ignore_value(virAsprintf(&ret, "s390: core='%d' psw-mask='0x%016llx' " - "psw-addr='0x%016llx' reason='%s'", + ignore_value(virAsprintf(&ret, "s390: reason='%s' core='%d' psw-mask='0x%016llx' " + "psw-addr='0x%016llx'", + info->data.s390.reason, info->data.s390.core, info->data.s390.psw_mask, - info->data.s390.psw_addr, - info->data.s390.reason)); + info->data.s390.psw_addr)); break; case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE: case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST: -- 2.17.0
On Wed, Jul 11, 2018 at 12:49:13 +0200, Bjoern Walk wrote:
This patch series introduces the ability to save additional information for the domain state and exposes this information in virsh domstate.
For example in the case of QEMU guest panic events, we can provide additional information like the crash reason or register state of the domain. This information usually gets logged in the domain log but for debugging it is useful to have it accessible from the client. Therefore, let's introduce a new public API function, virDomainGetStateParams, an extensible version of virDomainGetState, which returns the complete state of the domain, including newly introduced additional information.
Let's also extend virsh domstate and introduce a new parameter --info to show the domain state, reason and additional information when available.
virsh # domstate --info guest-1 crashed (panicked: disabled-wait core='1' psw-mask='0x000000000010f146' \ psw-addr='0x0002000180000000')
I was thinking you introduced a new API with typed parameters so that you can return each part of the info string as a separate parameter with a defined semantics. But I was wrong, you're just adding a new opaque string with more details about the reason. In that case typed parameters don't actually bring any additional value, they just complicate the usage of the API. The following prototype would be much better int virDomainGetState...(virDomainPtr domain, int *state, int *reason, char **info, unsigned int flags) On the other hand, is an opaque string really a good idea? It makes the additional info usable only for being shown to the user rather than being processed by an upper management layer. That's probably fine for crashed state, but perhaps other states would want to return something that is supposed to be processed. Maybe I'm just overthinking this, but I'd like to avoid having to add yet another API later. So the API could have the following prototype int virDomainGetStateParams(virDomainPtr domain, int *state, int *reason, virTypedParameterPtr *params, int *nparams, unsigned int flags) I'd still keep the state and reason parameters directly to make the API easier to use. Jirka
Jiri Denemark <jdenemar@redhat.com> [2018-07-11, 01:17PM +0200]:
On Wed, Jul 11, 2018 at 12:49:13 +0200, Bjoern Walk wrote:
This patch series introduces the ability to save additional information for the domain state and exposes this information in virsh domstate.
For example in the case of QEMU guest panic events, we can provide additional information like the crash reason or register state of the domain. This information usually gets logged in the domain log but for debugging it is useful to have it accessible from the client. Therefore, let's introduce a new public API function, virDomainGetStateParams, an extensible version of virDomainGetState, which returns the complete state of the domain, including newly introduced additional information.
Let's also extend virsh domstate and introduce a new parameter --info to show the domain state, reason and additional information when available.
virsh # domstate --info guest-1 crashed (panicked: disabled-wait core='1' psw-mask='0x000000000010f146' \ psw-addr='0x0002000180000000')
I was thinking you introduced a new API with typed parameters so that you can return each part of the info string as a separate parameter with a defined semantics. But I was wrong, you're just adding a new opaque string with more details about the reason. In that case typed parameters don't actually bring any additional value, they just complicate the usage of the API. The following prototype would be much better
The extensibility for this new API was more regarding for future additions of any state related information. Since libvirt does not have a deprecation model for APIs, whenever we would want to return additional information for the state (like in this case) a new API function has to be created.
int virDomainGetState...(virDomainPtr domain, int *state, int *reason, char **info, unsigned int flags)
On the other hand, is an opaque string really a good idea? It makes the additional info usable only for being shown to the user rather than being processed by an upper management layer. That's probably fine for crashed state, but perhaps other states would want to return something that is supposed to be processed. Maybe I'm just overthinking this, but I'd like to avoid having to add yet another API later. So the API could have the following prototype
Sure, if machine readability is a goal this approach makes certainly more sense. On the other hand, the same information can be queried by a qemu-monitor-command call and retrieved in JSON format. This information here is aimed at human interaction, similar to the log output. It is also unclear which platforms/hypervisors would provide what information and mapping all of them to a virTypedParameter entry could result in a rather large list. Certainly no arguments against your objection, and if the overall concensus is that this is something we want, I can definitely rework the API.
int virDomainGetStateParams(virDomainPtr domain, int *state, int *reason, virTypedParameterPtr *params, int *nparams, unsigned int flags)
I'd still keep the state and reason parameters directly to make the API easier to use.
Jirka
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- IBM Systems Linux on Z & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 ------------------------------------------------------------------------ Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Wed, Jul 11, 2018 at 01:40:31PM +0200, Bjoern Walk wrote:
Jiri Denemark <jdenemar@redhat.com> [2018-07-11, 01:17PM +0200]:
On Wed, Jul 11, 2018 at 12:49:13 +0200, Bjoern Walk wrote:
This patch series introduces the ability to save additional information for the domain state and exposes this information in virsh domstate.
For example in the case of QEMU guest panic events, we can provide additional information like the crash reason or register state of the domain. This information usually gets logged in the domain log but for debugging it is useful to have it accessible from the client. Therefore, let's introduce a new public API function, virDomainGetStateParams, an extensible version of virDomainGetState, which returns the complete state of the domain, including newly introduced additional information.
Let's also extend virsh domstate and introduce a new parameter --info to show the domain state, reason and additional information when available.
virsh # domstate --info guest-1 crashed (panicked: disabled-wait core='1' psw-mask='0x000000000010f146' \ psw-addr='0x0002000180000000')
I was thinking you introduced a new API with typed parameters so that you can return each part of the info string as a separate parameter with a defined semantics. But I was wrong, you're just adding a new opaque string with more details about the reason. In that case typed parameters don't actually bring any additional value, they just complicate the usage of the API. The following prototype would be much better
The extensibility for this new API was more regarding for future additions of any state related information. Since libvirt does not have a deprecation model for APIs, whenever we would want to return additional information for the state (like in this case) a new API function has to be created.
int virDomainGetState...(virDomainPtr domain, int *state, int *reason, char **info, unsigned int flags)
On the other hand, is an opaque string really a good idea? It makes the additional info usable only for being shown to the user rather than being processed by an upper management layer. That's probably fine for crashed state, but perhaps other states would want to return something that is supposed to be processed. Maybe I'm just overthinking this, but I'd like to avoid having to add yet another API later. So the API could have the following prototype
Sure, if machine readability is a goal this approach makes certainly more sense. On the other hand, the same information can be queried by a qemu-monitor-command call and retrieved in JSON format. This information here is aimed at human interaction, similar to the log output. It is also unclear which platforms/hypervisors would provide what information and mapping all of them to a virTypedParameter entry could result in a rather large list.
Certainly no arguments against your objection, and if the overall concensus is that this is something we want, I can definitely rework the API.
I'm not very particularly opinionated on this, but I think APIs should be machine-readable and CLI tools can convert that to human-readable format. You'll never know when a program will like to access that and having to tell anyone in the future that they need to parse a string is ugly IMHO. Also from the monitor you can get that information only if there is any QEMU running. I presume the state you are returning is saved somewhere along with the reason so that it can be provided later.
Martin Kletzander <mkletzan@redhat.com> [2018-07-13, 10:50AM +0200]:
I'm not very particularly opinionated on this, but I think APIs should be machine-readable and CLI tools can convert that to human-readable format. You'll never know when a program will like to access that and having to tell anyone in the future that they need to parse a string is ugly IMHO.
Ok, I can see the appeal for this. I agree that parsing the string is not optimal, it just didn't occur to me that we maybe want to have this information in other places as well. So then I would model the new API function like Jiri suggested: int virDomainGetStateParams(virDomainPtr domain, int *state, int *reason, virTypedParameterPtr *params, int *nparams, unsigned int flags) where each parameter in params holds a piece of the additional information, i.e VIR_DOMAIN_STATE_PARAMS_REASON_PSW_ADDR and so on on s390x. However, this raises the question how this would be handled on the client, who now receives an unknown set of parameters. How would for example virsh go on and reconstruct the info string? Just iterate of all returned parameters? Or should it introspect the parameters and perform an explicit formatting?
Also from the monitor you can get that information only if there is any QEMU running. I presume the state you are returning is saved somewhere along with the reason so that it can be provided later.
Yeah, in my QEMU-centric view I just assumed that this was an option. For where this information is saved, if I understand you correctly, I am not sure if I want to clutter this into the domain object where the state and reasons (and, in this patch series, the info) is saved. I try to find a way to retrieve this information from the hypervisor on the fly. I don't expect to much changes for the backend, so a general review of the code if I am heading in the right direction.would be appreciated.
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- IBM Systems Linux on Z & Virtualization Development ------------------------------------------------------------------------ IBM Deutschland Research & Development GmbH Schönaicher Str. 220, 71032 Böblingen Phone: +49 7031 16 1819 ------------------------------------------------------------------------ Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
participants (3)
-
Bjoern Walk -
Jiri Denemark -
Martin Kletzander