5:49 a.m.
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
5:49 a.m.
New subject: [libvirt] [PATCH 1/9] conf: add info to virDomainStateReason
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(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 2/9] conf: set/retrieve state information
Add the ability to set and retrieve additional state information for the
domain.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 3/9] lib: introduce virDomainGetStateParams function
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(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 4/9] remote: implement remoteDomainGetStateParams
Implement the API function virDomainGetStateParams for the remote driver
and wire up the remote protocol.
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 5/9] qemu: implement qemuDomainGetStateParams
Implement the API function virDomainGetStateParams for the QEMU
hypervisor driver.
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 6/9] qemu: set state information for guest panic event
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(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 7/9] virsh: domstate: report detailed state if available
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(a)linux.ibm.com>
Reviewed-by: Stefan Zimmermann <stzi(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 8/9] news: add entry for virDomainGetStateParams
Signed-off-by: Bjoern Walk <bwalk(a)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
5:49 a.m.
New subject: [libvirt] [PATCH 9/9] qemu: fix order of S390 panic event information
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(a)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
6:17 a.m.
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
6:40 a.m.
Jiri Denemark <jdenemar(a)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(a)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
3:50 a.m.
On Wed, Jul 11, 2018 at 01:40:31PM +0200, Bjoern Walk wrote:
Jiri Denemark <jdenemar(a)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.
3:25 a.m.
Martin Kletzander <mkletzan(a)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(a)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
2321
days inactive
2326
days old
13 comments
3 participants
participants (3)
-
Bjoern Walk -
Jiri Denemark -
Martin Kletzander