3:04 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 guest1
crashed (panicked: s390: core='1' psw-mask='0x1234000000000000' \
psw-addr='0x0000000000001234' reason='disabled-wait')
Previous version is here:
https://www.redhat.com/archives/libvir-list/2018-July/msg00686.html
v1 -> v2:
* refactored the public API according to discussions to provide a
more machine-parsable interface
Bjoern Walk (7):
qemu: monitor: move event data structure to domain
qemu: domain: store and update panic info
lib: introduce virDomainGetStateParams function
remote: implement remoteDomainGetStateParams
qemu: implement qemuDomainGetStateParams
virsh: domstate: report detailed state if available
news: add entry for virDomainGetStateParams
docs/news.xml | 11 +++
include/libvirt/libvirt-domain.h | 76 +++++++++++++++++
src/driver-hypervisor.h | 9 ++
src/libvirt-domain.c | 64 ++++++++++++++
src/libvirt_public.syms | 1 +
src/qemu/qemu_domain.c | 89 +++++++++++++++++++-
src/qemu/qemu_domain.h | 47 +++++++++++
src/qemu/qemu_driver.c | 126 +++++++++++++++++++++++++++-
src/qemu/qemu_monitor.c | 53 +-----------
src/qemu/qemu_monitor.h | 48 ++---------
src/qemu/qemu_monitor_json.c | 20 ++---
src/qemu/qemu_process.c | 2 +-
src/remote/remote_daemon_dispatch.c | 50 +++++++++++
src/remote/remote_driver.c | 44 ++++++++++
src/remote/remote_protocol.x | 22 ++++-
src/remote_protocol-structs | 12 +++
tools/virsh-domain-monitor.c | 102 +++++++++++++++++++---
tools/virsh.pod | 5 +-
18 files changed, 658 insertions(+), 123 deletions(-)
--
2.17.0
3:04 a.m.
New subject: [libvirt] [PATCH v2 1/7] qemu: monitor: move event data structure to domain
In order to later process the event data, we need to be able to access
it over the lifetime of the domain. So, as an initial step, let's move
the event data structure and utility functions to qemu_domain.[ch] and
rename them appropriately to make it accessible for other users.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/qemu/qemu_domain.c | 53 +++++++++++++++++++++++++++++++++++-
src/qemu/qemu_domain.h | 42 ++++++++++++++++++++++++++++
src/qemu/qemu_driver.c | 6 ++--
src/qemu/qemu_monitor.c | 53 +-----------------------------------
src/qemu/qemu_monitor.h | 48 ++++----------------------------
src/qemu/qemu_monitor_json.c | 20 +++++++-------
src/qemu/qemu_process.c | 2 +-
7 files changed, 114 insertions(+), 110 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 9ca4ca33..c6e07bcb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -13907,7 +13907,7 @@ qemuProcessEventFree(struct qemuProcessEvent *event)
switch (event->eventType) {
case QEMU_PROCESS_EVENT_GUESTPANIC:
- qemuMonitorEventPanicInfoFree(event->data);
+ qemuDomainStatePanicInfoFree(event->data);
break;
case QEMU_PROCESS_EVENT_RDMA_GID_STATUS_CHANGED:
qemuMonitorEventRdmaGidStatusFree(event->data);
@@ -14036,3 +14036,54 @@ qemuDomainNVRAMPathGenerate(virQEMUDriverConfigPtr cfg,
return 0;
}
+
+
+char *
+qemuDomainStatePanicInfoFormatMsg(qemuDomainStatePanicInfoPtr info)
+{
+ char *ret = NULL;
+
+ switch (info->type) {
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV:
+ ignore_value(virAsprintf(&ret,
+ "hyper-v: 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_DOMAIN_STATE_PANIC_INFO_TYPE_S390:
+ ignore_value(virAsprintf(&ret, "s390: core='%d'
psw-mask='0x%016llx' "
+ "psw-addr='0x%016llx'
reason='%s'",
+ info->data.s390.core,
+ info->data.s390.psw_mask,
+ info->data.s390.psw_addr,
+ info->data.s390.reason));
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_NONE:
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_LAST:
+ break;
+ }
+
+ return ret;
+}
+
+
+void
+qemuDomainStatePanicInfoFree(qemuDomainStatePanicInfoPtr info)
+{
+ if (!info)
+ return;
+
+ switch (info->type) {
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_S390:
+ VIR_FREE(info->data.s390.reason);
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_NONE:
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV:
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_LAST:
+ break;
+ }
+
+ VIR_FREE(info);
+}
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index ca24de15..145b377e 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -268,6 +268,48 @@ struct _qemuDomainSecretInfo {
} s;
};
+typedef enum {
+ QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_NONE = 0,
+ QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV,
+ QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_S390,
+
+ QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_LAST
+} qemuDomainStatePanicInfoType;
+
+typedef struct _qemuDomainStatePanicInfoHyperv qemuDomainStatePanicInfoHyperv;
+typedef qemuDomainStatePanicInfoHyperv *qemuDomainStatePanicInfoHypervPtr;
+struct _qemuDomainStatePanicInfoHyperv {
+ /* Hyper-V specific guest panic information (HV crash MSRs) */
+ unsigned long long arg1;
+ unsigned long long arg2;
+ unsigned long long arg3;
+ unsigned long long arg4;
+ unsigned long long arg5;
+};
+
+typedef struct _qemuDomainStatePanicInfoS390 qemuDomainStatePanicInfoS390;
+typedef qemuDomainStatePanicInfoS390 *qemuDomainStatePanicInfoS390Ptr;
+struct _qemuDomainStatePanicInfoS390 {
+ /* S390 specific guest panic information */
+ int core;
+ unsigned long long psw_mask;
+ unsigned long long psw_addr;
+ char *reason;
+};
+
+typedef struct _qemuDomainStatePanicInfo qemuDomainStatePanicInfo;
+typedef qemuDomainStatePanicInfo *qemuDomainStatePanicInfoPtr;
+struct _qemuDomainStatePanicInfo {
+ qemuDomainStatePanicInfoType type;
+ union {
+ qemuDomainStatePanicInfoHyperv hyperv;
+ qemuDomainStatePanicInfoS390 s390;
+ } data;
+};
+
+char *qemuDomainStatePanicInfoFormatMsg(qemuDomainStatePanicInfoPtr info);
+void qemuDomainStatePanicInfoFree(qemuDomainStatePanicInfoPtr info);
+
typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
struct _qemuDomainObjPrivate {
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index b27c6ce9..808d66cd 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4163,9 +4163,9 @@ doCoreDumpToAutoDumpPath(virQEMUDriverPtr driver,
static void
qemuProcessGuestPanicEventInfo(virQEMUDriverPtr driver,
virDomainObjPtr vm,
- qemuMonitorEventPanicInfoPtr info)
+ qemuDomainStatePanicInfoPtr info)
{
- char *msg = qemuMonitorGuestPanicEventInfoFormatMsg(info);
+ char *msg = qemuDomainStatePanicInfoFormatMsg(info);
char *timestamp = virTimeStringNow();
if (msg && timestamp)
@@ -4180,7 +4180,7 @@ static void
processGuestPanicEvent(virQEMUDriverPtr driver,
virDomainObjPtr vm,
int action,
- qemuMonitorEventPanicInfoPtr info)
+ qemuDomainStatePanicInfoPtr info)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
virObjectEventPtr event = NULL;
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index 0eb7f60e..9aa6172a 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -1389,7 +1389,7 @@ qemuMonitorEmitResume(qemuMonitorPtr mon)
int
qemuMonitorEmitGuestPanic(qemuMonitorPtr mon,
- qemuMonitorEventPanicInfoPtr info)
+ qemuDomainStatePanicInfoPtr info)
{
int ret = -1;
VIR_DEBUG("mon=%p", mon);
@@ -4296,57 +4296,6 @@ qemuMonitorQueryNamedBlockNodes(qemuMonitorPtr mon)
}
-char *
-qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info)
-{
- char *ret = NULL;
-
- switch (info->type) {
- case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_HYPERV:
- ignore_value(virAsprintf(&ret,
- "hyper-v: 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, "s390: core='%d'
psw-mask='0x%016llx' "
- "psw-addr='0x%016llx'
reason='%s'",
- info->data.s390.core,
- info->data.s390.psw_mask,
- info->data.s390.psw_addr,
- info->data.s390.reason));
- break;
- case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE:
- case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST:
- break;
- }
-
- return ret;
-}
-
-
-void
-qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info)
-{
- if (!info)
- return;
-
- switch (info->type) {
- case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390:
- VIR_FREE(info->data.s390.reason);
- break;
- case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE:
- case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_HYPERV:
- case QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST:
- break;
- }
-
- VIR_FREE(info);
-}
-
-
void
qemuMonitorEventRdmaGidStatusFree(qemuMonitorRdmaGidStatusPtr info)
{
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index fd7dcc91..49eb838b 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -39,6 +39,9 @@ typedef qemuMonitor *qemuMonitorPtr;
typedef struct _qemuMonitorMessage qemuMonitorMessage;
typedef qemuMonitorMessage *qemuMonitorMessagePtr;
+typedef struct _qemuDomainStatePanicInfo qemuDomainStatePanicInfo;
+typedef qemuDomainStatePanicInfo *qemuDomainStatePanicInfoPtr;
+
typedef int (*qemuMonitorPasswordHandler)(qemuMonitorPtr mon,
qemuMonitorMessagePtr msg,
const char *data,
@@ -67,45 +70,6 @@ struct _qemuMonitorMessage {
void *passwordOpaque;
};
-typedef enum {
- QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_NONE = 0,
- QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_HYPERV,
- QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390,
-
- QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_LAST
-} qemuMonitorEventPanicInfoType;
-
-typedef struct _qemuMonitorEventPanicInfoHyperv qemuMonitorEventPanicInfoHyperv;
-typedef qemuMonitorEventPanicInfoHyperv *qemuMonitorEventPanicInfoHypervPtr;
-struct _qemuMonitorEventPanicInfoHyperv {
- /* Hyper-V specific guest panic information (HV crash MSRs) */
- unsigned long long arg1;
- unsigned long long arg2;
- unsigned long long arg3;
- unsigned long long arg4;
- unsigned long long arg5;
-};
-
-typedef struct _qemuMonitorEventPanicInfoS390 qemuMonitorEventPanicInfoS390;
-typedef qemuMonitorEventPanicInfoS390 *qemuMonitorEventPanicInfoS390Ptr;
-struct _qemuMonitorEventPanicInfoS390 {
- /* S390 specific guest panic information */
- int core;
- unsigned long long psw_mask;
- unsigned long long psw_addr;
- char *reason;
-};
-
-typedef struct _qemuMonitorEventPanicInfo qemuMonitorEventPanicInfo;
-typedef qemuMonitorEventPanicInfo *qemuMonitorEventPanicInfoPtr;
-struct _qemuMonitorEventPanicInfo {
- qemuMonitorEventPanicInfoType type;
- union {
- qemuMonitorEventPanicInfoHyperv hyperv;
- qemuMonitorEventPanicInfoS390 s390;
- } data;
-};
-
typedef struct _qemuMonitorRdmaGidStatus qemuMonitorRdmaGidStatus;
typedef qemuMonitorRdmaGidStatus *qemuMonitorRdmaGidStatusPtr;
@@ -117,8 +81,6 @@ struct _qemuMonitorRdmaGidStatus {
};
-char *qemuMonitorGuestPanicEventInfoFormatMsg(qemuMonitorEventPanicInfoPtr info);
-void qemuMonitorEventPanicInfoFree(qemuMonitorEventPanicInfoPtr info);
void qemuMonitorEventRdmaGidStatusFree(qemuMonitorRdmaGidStatusPtr info);
typedef void (*qemuMonitorDestroyCallback)(qemuMonitorPtr mon,
@@ -209,7 +171,7 @@ typedef int (*qemuMonitorDomainPMSuspendDiskCallback)(qemuMonitorPtr
mon,
void *opaque);
typedef int (*qemuMonitorDomainGuestPanicCallback)(qemuMonitorPtr mon,
virDomainObjPtr vm,
- qemuMonitorEventPanicInfoPtr info,
+ qemuDomainStatePanicInfoPtr info,
void *opaque);
typedef int (*qemuMonitorDomainDeviceDeletedCallback)(qemuMonitorPtr mon,
virDomainObjPtr vm,
@@ -431,7 +393,7 @@ int qemuMonitorEmitBalloonChange(qemuMonitorPtr mon,
unsigned long long actual);
int qemuMonitorEmitPMSuspendDisk(qemuMonitorPtr mon);
int qemuMonitorEmitGuestPanic(qemuMonitorPtr mon,
- qemuMonitorEventPanicInfoPtr info);
+ qemuDomainStatePanicInfoPtr info);
int qemuMonitorEmitDeviceDeleted(qemuMonitorPtr mon,
const char *devAlias);
int qemuMonitorEmitNicRxFilterChanged(qemuMonitorPtr mon,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 5c16e1f3..d3fa371a 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -632,15 +632,15 @@ static void qemuMonitorJSONHandleResume(qemuMonitorPtr mon,
virJSONValuePtr data
}
-static qemuMonitorEventPanicInfoPtr
+static qemuDomainStatePanicInfoPtr
qemuMonitorJSONGuestPanicExtractInfoHyperv(virJSONValuePtr data)
{
- qemuMonitorEventPanicInfoPtr ret;
+ qemuDomainStatePanicInfoPtr ret;
if (VIR_ALLOC(ret) < 0)
return NULL;
- ret->type = QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_HYPERV;
+ ret->type = QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV;
if (virJSONValueObjectGetNumberUlong(data, "arg1",
&ret->data.hyperv.arg1) < 0 ||
virJSONValueObjectGetNumberUlong(data, "arg2",
&ret->data.hyperv.arg2) < 0 ||
@@ -655,14 +655,14 @@ qemuMonitorJSONGuestPanicExtractInfoHyperv(virJSONValuePtr data)
return ret;
error:
- qemuMonitorEventPanicInfoFree(ret);
+ qemuDomainStatePanicInfoFree(ret);
return NULL;
}
-static qemuMonitorEventPanicInfoPtr
+static qemuDomainStatePanicInfoPtr
qemuMonitorJSONGuestPanicExtractInfoS390(virJSONValuePtr data)
{
- qemuMonitorEventPanicInfoPtr ret;
+ qemuDomainStatePanicInfoPtr ret;
int core;
unsigned long long psw_mask, psw_addr;
const char *reason = NULL;
@@ -670,7 +670,7 @@ qemuMonitorJSONGuestPanicExtractInfoS390(virJSONValuePtr data)
if (VIR_ALLOC(ret) < 0)
return NULL;
- ret->type = QEMU_MONITOR_EVENT_PANIC_INFO_TYPE_S390;
+ ret->type = QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_S390;
if (virJSONValueObjectGetNumberInt(data, "core", &core) < 0 ||
virJSONValueObjectGetNumberUlong(data, "psw-mask", &psw_mask) <
0 ||
@@ -690,11 +690,11 @@ qemuMonitorJSONGuestPanicExtractInfoS390(virJSONValuePtr data)
return ret;
error:
- qemuMonitorEventPanicInfoFree(ret);
+ qemuDomainStatePanicInfoFree(ret);
return NULL;
}
-static qemuMonitorEventPanicInfoPtr
+static qemuDomainStatePanicInfoPtr
qemuMonitorJSONGuestPanicExtractInfo(virJSONValuePtr data)
{
const char *type = virJSONValueObjectGetString(data, "type");
@@ -715,7 +715,7 @@ qemuMonitorJSONHandleGuestPanic(qemuMonitorPtr mon,
virJSONValuePtr data)
{
virJSONValuePtr infojson = virJSONValueObjectGetObject(data, "info");
- qemuMonitorEventPanicInfoPtr info = NULL;
+ qemuDomainStatePanicInfoPtr info = NULL;
if (infojson)
info = qemuMonitorJSONGuestPanicExtractInfo(infojson);
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
index dc7317b7..aec4af8f 100644
--- a/src/qemu/qemu_process.c
+++ b/src/qemu/qemu_process.c
@@ -1259,7 +1259,7 @@ qemuProcessHandlePMSuspendDisk(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
static int
qemuProcessHandleGuestPanic(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
virDomainObjPtr vm,
- qemuMonitorEventPanicInfoPtr info,
+ qemuDomainStatePanicInfoPtr info,
void *opaque)
{
virQEMUDriverPtr driver = opaque;
--
2.17.0
3:04 a.m.
New subject: [libvirt] [PATCH v2 2/7] qemu: domain: store and update panic info
Let's store additional state information in the hypervisor-specific
private data to virDomainObj. For now, just consider panic state in QEMU
domains for which additional information is available from the guest
crash event handler.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/qemu/qemu_domain.c | 36 ++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 5 +++++
src/qemu/qemu_driver.c | 2 ++
3 files changed, 43 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c6e07bcb..fb43035e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2052,6 +2052,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv)
virBitmapFree(priv->migrationCaps);
priv->migrationCaps = NULL;
+ qemuDomainStatePanicInfoFree(priv->panicInfo);
+ priv->panicInfo = NULL;
+
qemuDomainObjResetJob(priv);
qemuDomainObjResetAsyncJob(priv);
}
@@ -14069,6 +14072,39 @@ qemuDomainStatePanicInfoFormatMsg(qemuDomainStatePanicInfoPtr
info)
}
+void
+qemuDomainStatePanicInfoSet(virDomainObjPtr vm,
+ qemuDomainStatePanicInfoPtr info)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+
+ if (!priv->panicInfo && VIR_ALLOC(priv->panicInfo) < 0)
+ return;
+
+ priv->panicInfo->type = info->type;
+
+ switch (info->type) {
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV:
+ priv->panicInfo->data.hyperv.arg1 = info->data.hyperv.arg1;
+ priv->panicInfo->data.hyperv.arg2 = info->data.hyperv.arg2;
+ priv->panicInfo->data.hyperv.arg3 = info->data.hyperv.arg3;
+ priv->panicInfo->data.hyperv.arg4 = info->data.hyperv.arg4;
+ priv->panicInfo->data.hyperv.arg5 = info->data.hyperv.arg5;
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_S390:
+ priv->panicInfo->data.s390.core = info->data.s390.core;
+ priv->panicInfo->data.s390.psw_mask = info->data.s390.psw_mask;
+ priv->panicInfo->data.s390.psw_addr = info->data.s390.psw_addr;
+ ignore_value(VIR_STRDUP(priv->panicInfo->data.s390.reason,
+ info->data.s390.reason));
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_NONE:
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_LAST:
+ break;
+ }
+}
+
+
void
qemuDomainStatePanicInfoFree(qemuDomainStatePanicInfoPtr info)
{
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
index 145b377e..aa7457b1 100644
--- a/src/qemu/qemu_domain.h
+++ b/src/qemu/qemu_domain.h
@@ -308,6 +308,8 @@ struct _qemuDomainStatePanicInfo {
};
char *qemuDomainStatePanicInfoFormatMsg(qemuDomainStatePanicInfoPtr info);
+void qemuDomainStatePanicInfoSet(virDomainObjPtr vm,
+ qemuDomainStatePanicInfoPtr info);
void qemuDomainStatePanicInfoFree(qemuDomainStatePanicInfoPtr info);
typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
@@ -417,6 +419,9 @@ struct _qemuDomainObjPrivate {
/* true if global -mem-prealloc appears on cmd line */
bool memPrealloc;
+
+ /* store extra information for panicked domain state */
+ qemuDomainStatePanicInfoPtr panicInfo;
};
# define QEMU_DOMAIN_PRIVATE(vm) \
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 808d66cd..16e34a98 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4171,6 +4171,8 @@ qemuProcessGuestPanicEventInfo(virQEMUDriverPtr driver,
if (msg && timestamp)
qemuDomainLogAppendMessage(driver, vm, "%s: panic %s\n", timestamp,
msg);
+ qemuDomainStatePanicInfoSet(vm, info);
+
VIR_FREE(timestamp);
VIR_FREE(msg);
}
--
2.17.0
8:05 a.m.
New subject: [libvirt] [PATCH v2 2/7] qemu: domain: store and update panic info
On 3/25/19 9:04 AM, Bjoern Walk wrote:
Let's store additional state information in the
hypervisor-specific
private data to virDomainObj. For now, just consider panic state in QEMU
domains for which additional information is available from the guest
crash event handler.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/qemu/qemu_domain.c | 36 ++++++++++++++++++++++++++++++++++++
src/qemu/qemu_domain.h | 5 +++++
src/qemu/qemu_driver.c | 2 ++
3 files changed, 43 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index c6e07bcb..fb43035e 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2052,6 +2052,9 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv)
virBitmapFree(priv->migrationCaps);
priv->migrationCaps = NULL;
+ qemuDomainStatePanicInfoFree(priv->panicInfo);
+ priv->panicInfo = NULL;
+
qemuDomainObjResetJob(priv);
qemuDomainObjResetAsyncJob(priv);
}
@@ -14069,6 +14072,39 @@ qemuDomainStatePanicInfoFormatMsg(qemuDomainStatePanicInfoPtr
info)
}
+void
+qemuDomainStatePanicInfoSet(virDomainObjPtr vm,
+ qemuDomainStatePanicInfoPtr info)
+{
+ qemuDomainObjPrivatePtr priv = vm->privateData;
+
+ if (!priv->panicInfo && VIR_ALLOC(priv->panicInfo) < 0)
+ return;
+
+ priv->panicInfo->type = info->type;
+
+ switch (info->type) {
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV:
+ priv->panicInfo->data.hyperv.arg1 = info->data.hyperv.arg1;
+ priv->panicInfo->data.hyperv.arg2 = info->data.hyperv.arg2;
+ priv->panicInfo->data.hyperv.arg3 = info->data.hyperv.arg3;
+ priv->panicInfo->data.hyperv.arg4 = info->data.hyperv.arg4;
+ priv->panicInfo->data.hyperv.arg5 = info->data.hyperv.arg5;
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_S390:
+ priv->panicInfo->data.s390.core = info->data.s390.core;
+ priv->panicInfo->data.s390.psw_mask = info->data.s390.psw_mask;
+ priv->panicInfo->data.s390.psw_addr = info->data.s390.psw_addr;
+ ignore_value(VIR_STRDUP(priv->panicInfo->data.s390.reason,
+ info->data.s390.reason));
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_NONE:
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_LAST:
+ break;
+ }
How about memcpy() and then just STRDUP() in the one place we need to?
No need to send v3 for that, just asking and if so, I can fix that
before pushing.
My reasoning is that it might be easy to forget update this function if
qemuDomainStatePanicInfo struct gets a new member.
Michal
8:17 a.m.
New subject: [libvirt] [PATCH v2 2/7] qemu: domain: store and update panic info
Michal Privoznik <mprivozn(a)redhat.com> [2019-03-27, 02:05PM +0100]:
On 3/25/19 9:04 AM, Bjoern Walk wrote:
> @@ -14069,6 +14072,39 @@
qemuDomainStatePanicInfoFormatMsg(qemuDomainStatePanicInfoPtr info)
> }
> +void
> +qemuDomainStatePanicInfoSet(virDomainObjPtr vm,
> + qemuDomainStatePanicInfoPtr info)
> +{
> + qemuDomainObjPrivatePtr priv = vm->privateData;
> +
> + if (!priv->panicInfo && VIR_ALLOC(priv->panicInfo) < 0)
> + return;
> +
> + priv->panicInfo->type = info->type;
> +
> + switch (info->type) {
> + case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV:
> + priv->panicInfo->data.hyperv.arg1 = info->data.hyperv.arg1;
> + priv->panicInfo->data.hyperv.arg2 = info->data.hyperv.arg2;
> + priv->panicInfo->data.hyperv.arg3 = info->data.hyperv.arg3;
> + priv->panicInfo->data.hyperv.arg4 = info->data.hyperv.arg4;
> + priv->panicInfo->data.hyperv.arg5 = info->data.hyperv.arg5;
> + break;
> + case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_S390:
> + priv->panicInfo->data.s390.core = info->data.s390.core;
> + priv->panicInfo->data.s390.psw_mask = info->data.s390.psw_mask;
> + priv->panicInfo->data.s390.psw_addr = info->data.s390.psw_addr;
> + ignore_value(VIR_STRDUP(priv->panicInfo->data.s390.reason,
> + info->data.s390.reason));
> + break;
> + case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_NONE:
> + case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_LAST:
> + break;
> + }
How about memcpy() and then just STRDUP() in the one place we need to?
No need to send v3 for that, just asking and if so, I can fix that before
pushing.
My reasoning is that it might be easy to forget update this function if
qemuDomainStatePanicInfo struct gets a new member.
Yes, I agree, that's much better.
Michal
3:04 a.m.
New subject: [libvirt] [PATCH v2 3/7] lib: introduce virDomainGetStateParams function
This API function extends the virDomainGetState function by returning
additional state information as a dictionary of typed parameters.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
include/libvirt/libvirt-domain.h | 76 ++++++++++++++++++++++++++++++++
src/driver-hypervisor.h | 9 ++++
src/libvirt-domain.c | 64 +++++++++++++++++++++++++++
src/libvirt_public.syms | 1 +
4 files changed, 150 insertions(+)
diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
index 1d5bdb54..79ebcba2 100644
--- a/include/libvirt/libvirt-domain.h
+++ b/include/libvirt/libvirt-domain.h
@@ -1278,6 +1278,82 @@ int virDomainGetState (virDomainPtr
domain,
int *reason,
unsigned int flags);
+typedef enum {
+ VIR_DOMAIN_STATE_INFO_TYPE_NONE,
+ VIR_DOMAIN_STATE_INFO_TYPE_QEMU_HYPERV,
+ VIR_DOMAIN_STATE_INFO_TYPE_QEMU_S390,
+
+ VIR_DOMAIN_STATE_INFO_TYPE_LAST
+} virDomainStateInfoType;
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_INFO_TYPE:
+ * specifies the type of additional state information available
+ */
+# define VIR_DOMAIN_STATE_PARAMS_INFO_TYPE "info_type"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG1:
+ * Hyper-V-specific panic state information: HV crash MSR1
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG1 "panic_hyperv_msr1"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG2:
+ * Hyper-V-specific panic state information: HV crash MSR2
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG2 "panic_hyperv_msr2"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG3:
+ * Hyper-V-specific panic state information: HV crash MSR3
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG3 "panic_hyperv_msr3"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG4:
+ * Hyper-V-specific panic state information: HV crash MSR4
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG4 "panic_hyperv_msr4"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG5:
+ * Hyper-V-specific panic state information: HV crash MSR5
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG5 "panic_hyperv_msr5"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE:
+ * S390-specific panic state information: panicked core
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE "panic_s390_core"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK:
+ * S390-specific panic state information: PSW mask
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK
"panic_s390_psw_mask"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR:
+ * S390-specific panic state information: PSW address
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR
"panic_s390_psw_addr"
+
+/**
+ * VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON:
+ * S390-specific panic state information: human-readable reason
+ */
+# define VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON
"panic_s390_reason"
+
+int virDomainGetStateParams(virDomainPtr domain,
+ int *state,
+ int *reason,
+ 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 5315e33d..fd6ed690 100644
--- a/src/driver-hypervisor.h
+++ b/src/driver-hypervisor.h
@@ -220,6 +220,14 @@ typedef int
int *reason,
unsigned int flags);
+typedef int
+(*virDrvDomainGetStateParams)(virDomainPtr domain,
+ int *state,
+ int *reason,
+ virTypedParameterPtr *params,
+ int *nparams,
+ unsigned int flags);
+
typedef int
(*virDrvDomainGetControlInfo)(virDomainPtr domain,
virDomainControlInfoPtr info,
@@ -1390,6 +1398,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 be5b1f67..349c8498 100644
--- a/src/libvirt-domain.c
+++ b/src/libvirt-domain.c
@@ -2502,6 +2502,70 @@ virDomainGetState(virDomainPtr domain,
}
+/**
+ * virDomainGetStateParams:
+ * @domain: a domain object
+ * @state: returned state of the domain (one of virDomainState)
+ * @reason: returned reason which led to @state (one of virDomain*Reason)
+ * @params: where to store additional state information, 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,
+ int *state,
+ int *reason,
+ 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 (VIR_DRV_SUPPORTS_FEATURE(conn->driver, conn,
+ VIR_DRV_FEATURE_TYPED_PARAM_STRING))
+ flags |= VIR_TYPED_PARAM_STRING_OKAY;
+
+ if (conn->driver->domainGetStateParams) {
+ int ret;
+ ret = conn->driver->domainGetStateParams(domain, state, reason,
+ 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 dbce3336..9e6d473a 100644
--- a/src/libvirt_public.syms
+++ b/src/libvirt_public.syms
@@ -817,6 +817,7 @@ LIBVIRT_4.10.0 {
LIBVIRT_5.2.0 {
global:
virConnectGetStoragePoolCapabilities;
+ virDomainGetStateParams;
} LIBVIRT_4.10.0;
# .... define new API here using predicted next version number ....
--
2.17.0
3:04 a.m.
New subject: [libvirt] [PATCH v2 4/7] remote: implement remoteDomainGetStateParams
Implement the API function virDomainGetStateParams for the remote driver
and wire up the remote protocol.
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 | 50 +++++++++++++++++++++++++++++
src/remote/remote_driver.c | 44 +++++++++++++++++++++++++
src/remote/remote_protocol.x | 22 ++++++++++++-
src/remote_protocol-structs | 12 +++++++
4 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index df282590..e1e48559 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -7392,3 +7392,53 @@ remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
}
return -1;
}
+
+static int
+remoteDispatchDomainGetStateParams(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
+ remote_domain_get_state_params_args *args,
+ remote_domain_get_state_params_ret *ret)
+{
+ virDomainPtr dom = NULL;
+ virTypedParameterPtr params = NULL;
+ int nparams = 0;
+ int rv = -1;
+ struct daemonClientPrivate *priv =
+ virNetServerClientGetPrivateData(client);
+
+ if (!priv->conn) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
+ goto cleanup;
+ }
+
+ if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+ goto cleanup;
+
+ if (virDomainGetStateParams(dom, &ret->state, &ret->reason,
¶ms,
+ &nparams, args->flags) < 0)
+ goto cleanup;
+
+ if (nparams > REMOTE_DOMAIN_STATE_PARAMS_MAX) {
+ virReportError(VIR_ERR_RPC,
+ _("Too many state information entries '%d' for limit
'%d'"),
+ nparams, REMOTE_DOMAIN_STATE_PARAMS_MAX);
+ goto cleanup;
+ }
+
+ if (virTypedParamsSerialize(params, nparams,
+ (virTypedParameterRemotePtr *)
&ret->params.params_val,
+ &ret->params.params_len,
+ VIR_TYPED_PARAM_STRING_OKAY) < 0)
+ goto cleanup;
+
+ rv = 0;
+
+ cleanup:
+ if (rv < 0)
+ virNetMessageSaveError(rerr);
+ virTypedParamsFree(params, nparams);
+ virObjectUnref(dom);
+ return rv;
+}
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 5c4dd412..e0962014 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8131,6 +8131,49 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol,
return rv;
}
+static int
+remoteDomainGetStateParams(virDomainPtr domain,
+ int *state,
+ int *reason,
+ virTypedParameterPtr *params,
+ int *nparams,
+ unsigned int flags)
+{
+ int rv = -1;
+ remote_domain_get_state_params_args args;
+ remote_domain_get_state_params_ret ret;
+ struct private_data *priv = domain->conn->privateData;
+
+ remoteDriverLock(priv);
+
+ make_nonnull_domain(&args.dom, domain);
+ args.flags = flags;
+
+ memset(&ret, 0, sizeof(ret));
+ if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_STATE_PARAMS,
+ (xdrproc_t) xdr_remote_domain_get_state_params_args, (char *) &args,
+ (xdrproc_t) xdr_remote_domain_get_state_params_ret, (char *) &ret) ==
-1)
+ goto done;
+
+ *state = ret.state;
+ *reason = ret.reason;
+
+ if (virTypedParamsDeserialize((virTypedParameterRemotePtr) ret.params.params_val,
+ ret.params.params_len,
+ REMOTE_DOMAIN_STATE_PARAMS_MAX,
+ params, nparams) < 0)
+ goto cleanup;
+
+ rv = 0;
+
+ cleanup:
+ xdr_free((xdrproc_t) xdr_remote_domain_get_state_params_ret,
+ (char *) &ret);
+ done:
+ remoteDriverUnlock(priv);
+ return rv;
+}
+
/* get_nonnull_domain and get_nonnull_network turn an on-wire
* (name, uuid) pair into virDomainPtr or virNetworkPtr object.
@@ -8326,6 +8369,7 @@ static virHypervisorDriver hypervisor_driver = {
.domainSetPerfEvents = remoteDomainSetPerfEvents, /* 1.3.3 */
.domainGetInfo = remoteDomainGetInfo, /* 0.3.0 */
.domainGetState = remoteDomainGetState, /* 0.9.2 */
+ .domainGetStateParams = remoteDomainGetStateParams, /* TODO: 5.2.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 74be4b37..6db1ccc1 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -263,6 +263,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 information entries */
+const REMOTE_DOMAIN_STATE_PARAMS_MAX = 16;
+
/* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */
typedef opaque remote_uuid[VIR_UUID_BUFLEN];
@@ -2789,6 +2792,17 @@ 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 {
+ int state;
+ int reason;
+ remote_typed_param params<REMOTE_DOMAIN_STATE_PARAMS_MAX>;
+};
+
struct remote_domain_migrate_begin3_args {
remote_nonnull_domain dom;
remote_string xmlin;
@@ -6342,5 +6356,11 @@ enum remote_procedure {
* @generate: both
* @acl: connect:read
*/
- REMOTE_PROC_CONNECT_GET_STORAGE_POOL_CAPABILITIES = 403
+ REMOTE_PROC_CONNECT_GET_STORAGE_POOL_CAPABILITIES = 403,
+
+ /**
+ * @generate: none
+ * @acl: domain:read
+ */
+ REMOTE_PROC_DOMAIN_GET_STATE_PARAMS = 404
};
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 768189c5..670164de 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2162,6 +2162,18 @@ 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 {
+ int state;
+ int reason;
+ struct {
+ u_int params_len;
+ remote_typed_param * params_val;
+ } params;
+};
struct remote_domain_migrate_begin3_args {
remote_nonnull_domain dom;
remote_string xmlin;
--
2.17.0
8:05 a.m.
New subject: [libvirt] [PATCH v2 4/7] remote: implement remoteDomainGetStateParams
On 3/25/19 9:04 AM, Bjoern Walk wrote:
Implement the API function virDomainGetStateParams for the remote
driver
and wire up the remote protocol.
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 | 50 +++++++++++++++++++++++++++++
src/remote/remote_driver.c | 44 +++++++++++++++++++++++++
src/remote/remote_protocol.x | 22 ++++++++++++-
src/remote_protocol-structs | 12 +++++++
4 files changed, 127 insertions(+), 1 deletion(-)
diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c
index df282590..e1e48559 100644
--- a/src/remote/remote_daemon_dispatch.c
+++ b/src/remote/remote_daemon_dispatch.c
@@ -7392,3 +7392,53 @@ remoteSerializeDomainDiskErrors(virDomainDiskErrorPtr errors,
}
return -1;
}
+
+static int
+remoteDispatchDomainGetStateParams(virNetServerPtr server ATTRIBUTE_UNUSED,
+ virNetServerClientPtr client,
+ virNetMessagePtr msg ATTRIBUTE_UNUSED,
+ virNetMessageErrorPtr rerr,
+ remote_domain_get_state_params_args *args,
+ remote_domain_get_state_params_ret *ret)
+{
+ virDomainPtr dom = NULL;
+ virTypedParameterPtr params = NULL;
+ int nparams = 0;
+ int rv = -1;
+ struct daemonClientPrivate *priv =
+ virNetServerClientGetPrivateData(client);
+
+ if (!priv->conn) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not
open"));
+ goto cleanup;
+ }
+
+ if (!(dom = get_nonnull_domain(priv->conn, args->dom)))
+ goto cleanup;
+
+ if (virDomainGetStateParams(dom, &ret->state, &ret->reason,
¶ms,
+ &nparams, args->flags) < 0)
+ goto cleanup;
+
+ if (nparams > REMOTE_DOMAIN_STATE_PARAMS_MAX) {
+ virReportError(VIR_ERR_RPC,
+ _("Too many state information entries '%d' for limit
'%d'"),
+ nparams, REMOTE_DOMAIN_STATE_PARAMS_MAX);
+ goto cleanup;
+ }
+
+ if (virTypedParamsSerialize(params, nparams,
+ (virTypedParameterRemotePtr *)
&ret->params.params_val,
+ &ret->params.params_len,
+ VIR_TYPED_PARAM_STRING_OKAY) < 0)
+ goto cleanup;
+
+ rv = 0;
+
+ cleanup:
+ if (rv < 0)
+ virNetMessageSaveError(rerr);
+ virTypedParamsFree(params, nparams);
+ virObjectUnref(dom);
+ return rv;
+}
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 5c4dd412..e0962014 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -8131,6 +8131,49 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol,
return rv;
}
+static int
+remoteDomainGetStateParams(virDomainPtr domain,
+ int *state,
+ int *reason,
+ virTypedParameterPtr *params,
+ int *nparams,
+ unsigned int flags)
+{
+ int rv = -1;
+ remote_domain_get_state_params_args args;
+ remote_domain_get_state_params_ret ret;
+ struct private_data *priv = domain->conn->privateData;
+
+ remoteDriverLock(priv);
+
+ make_nonnull_domain(&args.dom, domain);
+ args.flags = flags;
+
+ memset(&ret, 0, sizeof(ret));
+ if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_STATE_PARAMS,
+ (xdrproc_t) xdr_remote_domain_get_state_params_args, (char *) &args,
+ (xdrproc_t) xdr_remote_domain_get_state_params_ret, (char *) &ret) ==
-1)
+ goto done;
+
+ *state = ret.state;
+ *reason = ret.reason;
Neither @state nor @reason is required to be non-NULL. I mean, based on
checks from 3/7 it's just fine to call virDomaingetStateParams(dom,
NULL, NULL, ..);
+
+ if (virTypedParamsDeserialize((virTypedParameterRemotePtr) ret.params.params_val,
+ ret.params.params_len,
+ REMOTE_DOMAIN_STATE_PARAMS_MAX,
+ params, nparams) < 0)
+ goto cleanup;
+
+ rv = 0;
+
+ cleanup:
+ xdr_free((xdrproc_t) xdr_remote_domain_get_state_params_ret,
+ (char *) &ret);
+ done:
+ remoteDriverUnlock(priv);
+ return rv;
+}
+
/* get_nonnull_domain and get_nonnull_network turn an on-wire
* (name, uuid) pair into virDomainPtr or virNetworkPtr object.
@@ -8326,6 +8369,7 @@ static virHypervisorDriver hypervisor_driver = {
.domainSetPerfEvents = remoteDomainSetPerfEvents, /* 1.3.3 */
.domainGetInfo = remoteDomainGetInfo, /* 0.3.0 */
.domainGetState = remoteDomainGetState, /* 0.9.2 */
+ .domainGetStateParams = remoteDomainGetStateParams, /* TODO: 5.2.0 */
No need to mark it as TODO. It's okay to say these patches target 5.2.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 74be4b37..6db1ccc1 100644
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -263,6 +263,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 information entries */
+const REMOTE_DOMAIN_STATE_PARAMS_MAX = 16;
+
/* UUID. VIR_UUID_BUFLEN definition comes from libvirt.h */
typedef opaque remote_uuid[VIR_UUID_BUFLEN];
@@ -2789,6 +2792,17 @@ 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 {
+ int state;
+ int reason;
+ remote_typed_param params<REMOTE_DOMAIN_STATE_PARAMS_MAX>;
+};
+
struct remote_domain_migrate_begin3_args {
remote_nonnull_domain dom;
remote_string xmlin;
@@ -6342,5 +6356,11 @@ enum remote_procedure {
* @generate: both
* @acl: connect:read
*/
- REMOTE_PROC_CONNECT_GET_STORAGE_POOL_CAPABILITIES = 403
+ REMOTE_PROC_CONNECT_GET_STORAGE_POOL_CAPABILITIES = 403,
+
+ /**
+ * @generate: none
+ * @acl: domain:read
+ */
+ REMOTE_PROC_DOMAIN_GET_STATE_PARAMS = 404
};
diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index 768189c5..670164de 100644
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -2162,6 +2162,18 @@ 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 {
+ int state;
+ int reason;
+ struct {
+ u_int params_len;
+ remote_typed_param * params_val;
+ } params;
+};
struct remote_domain_migrate_begin3_args {
remote_nonnull_domain dom;
remote_string xmlin;
This is missing REMOTE_PROC_DOMAIN_GET_STATE_PARAMS = 404, at EOF :-)
Michal
8:19 a.m.
New subject: [libvirt] [PATCH v2 4/7] remote: implement remoteDomainGetStateParams
Michal Privoznik <mprivozn(a)redhat.com> [2019-03-27, 02:05PM +0100]:
On 3/25/19 9:04 AM, Bjoern Walk wrote:
> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
> index 5c4dd412..e0962014 100644
> --- a/src/remote/remote_driver.c
> +++ b/src/remote/remote_driver.c
> @@ -8131,6 +8131,49 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol,
> return rv;
> }
> +static int
> +remoteDomainGetStateParams(virDomainPtr domain,
> + int *state,
> + int *reason,
> + virTypedParameterPtr *params,
> + int *nparams,
> + unsigned int flags)
> +{
> + int rv = -1;
> + remote_domain_get_state_params_args args;
> + remote_domain_get_state_params_ret ret;
> + struct private_data *priv = domain->conn->privateData;
> +
> + remoteDriverLock(priv);
> +
> + make_nonnull_domain(&args.dom, domain);
> + args.flags = flags;
> +
> + memset(&ret, 0, sizeof(ret));
> + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_STATE_PARAMS,
> + (xdrproc_t) xdr_remote_domain_get_state_params_args, (char *)
&args,
> + (xdrproc_t) xdr_remote_domain_get_state_params_ret, (char *) &ret)
== -1)
> + goto done;
> +
> + *state = ret.state;
> + *reason = ret.reason;
Neither @state nor @reason is required to be non-NULL. I mean, based on
checks from 3/7 it's just fine to call virDomaingetStateParams(dom, NULL,
NULL, ..);
Yepp, correct. Missed that.
> diff --git a/src/remote_protocol-structs
b/src/remote_protocol-structs
> index 768189c5..670164de 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -2162,6 +2162,18 @@ 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 {
> + int state;
> + int reason;
> + struct {
> + u_int params_len;
> + remote_typed_param * params_val;
> + } params;
> +};
> struct remote_domain_migrate_begin3_args {
> remote_nonnull_domain dom;
> remote_string xmlin;
>
This is missing REMOTE_PROC_DOMAIN_GET_STATE_PARAMS = 404, at EOF :-)
Huh? Never even noticed. And yet it compiles and runs just fine. Where
are those definitions used?
Michal
8:44 a.m.
New subject: [libvirt] [PATCH v2 4/7] remote: implement remoteDomainGetStateParams
On 3/27/19 2:19 PM, Bjoern Walk wrote:
Michal Privoznik <mprivozn(a)redhat.com> [2019-03-27, 02:05PM
+0100]:
> On 3/25/19 9:04 AM, Bjoern Walk wrote:
>> diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
>> index 5c4dd412..e0962014 100644
>> --- a/src/remote/remote_driver.c
>> +++ b/src/remote/remote_driver.c
>> @@ -8131,6 +8131,49 @@ remoteStorageVolGetInfoFlags(virStorageVolPtr vol,
>> return rv;
>> }
>> +static int
>> +remoteDomainGetStateParams(virDomainPtr domain,
>> + int *state,
>> + int *reason,
>> + virTypedParameterPtr *params,
>> + int *nparams,
>> + unsigned int flags)
>> +{
>> + int rv = -1;
>> + remote_domain_get_state_params_args args;
>> + remote_domain_get_state_params_ret ret;
>> + struct private_data *priv = domain->conn->privateData;
>> +
>> + remoteDriverLock(priv);
>> +
>> + make_nonnull_domain(&args.dom, domain);
>> + args.flags = flags;
>> +
>> + memset(&ret, 0, sizeof(ret));
>> + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_STATE_PARAMS,
>> + (xdrproc_t) xdr_remote_domain_get_state_params_args, (char *)
&args,
>> + (xdrproc_t) xdr_remote_domain_get_state_params_ret, (char *)
&ret) == -1)
>> + goto done;
>> +
>> + *state = ret.state;
>> + *reason = ret.reason;
>
> Neither @state nor @reason is required to be non-NULL. I mean, based on
> checks from 3/7 it's just fine to call virDomaingetStateParams(dom, NULL,
> NULL, ..);
Yepp, correct. Missed that.
>> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
>> index 768189c5..670164de 100644
>> --- a/src/remote_protocol-structs
>> +++ b/src/remote_protocol-structs
>> @@ -2162,6 +2162,18 @@ 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 {
>> + int state;
>> + int reason;
>> + struct {
>> + u_int params_len;
>> + remote_typed_param * params_val;
>> + } params;
>> +};
>> struct remote_domain_migrate_begin3_args {
>> remote_nonnull_domain dom;
>> remote_string xmlin;
>>
>
> This is missing REMOTE_PROC_DOMAIN_GET_STATE_PARAMS = 404, at EOF :-)
Huh? Never even noticed. And yet it compiles and runs just fine. Where
are those definitions used?
This is a file that's used in 'make check' and you need pdwtags to run
it. If you don't have it installed the test is skipped.
Michal
3:04 a.m.
New subject: [libvirt] [PATCH v2 5/7] qemu: implement qemuDomainGetStateParams
Implement the API function virDomainGetStateParams for the QEMU
hypervisor driver.
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
src/qemu/qemu_driver.c | 118 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 118 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 16e34a98..4af82572 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -2641,6 +2641,123 @@ qemuDomainGetState(virDomainPtr dom,
return ret;
}
+static int
+qemuDomainGetStateParams(virDomainPtr dom,
+ int *state,
+ int *reason,
+ virTypedParameterPtr *params,
+ int *nparams,
+ unsigned int flags)
+{
+ int ret = -1;
+ virDomainObjPtr vm;
+ qemuDomainStatePanicInfoPtr info = NULL;
+ virTypedParameterPtr par = NULL;
+ int npar = 0, maxparams = 0;
+
+ virCheckFlags(VIR_TYPED_PARAM_STRING_OKAY, -1);
+
+ if (!(vm = qemuDomObjFromDomain(dom)))
+ goto cleanup;
+
+ if (virDomainGetStateParamsEnsureACL(dom->conn, vm->def) < 0)
+ goto cleanup;
+
+ *state = virDomainObjGetState(vm, reason);
+
+ info = QEMU_DOMAIN_PRIVATE(vm)->panicInfo;
+
+ if (!info) {
+ if (virTypedParamsAddInt(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_INFO_TYPE,
+ VIR_DOMAIN_STATE_INFO_TYPE_NONE) < 0) {
+ goto cleanup;
+ }
+ } else if (*state == VIR_DOMAIN_CRASHED &&
+ *reason == VIR_DOMAIN_CRASHED_PANICKED) {
+ switch (info->type) {
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_HYPERV:
+ if (virTypedParamsAddInt(&par, &npar,
+ &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_INFO_TYPE,
+ VIR_DOMAIN_STATE_INFO_TYPE_QEMU_HYPERV) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddULLong(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG1,
+ info->data.hyperv.arg1) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddULLong(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG2,
+ info->data.hyperv.arg2) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddULLong(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG3,
+ info->data.hyperv.arg3) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddULLong(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG4,
+ info->data.hyperv.arg4) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddULLong(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG5,
+ info->data.hyperv.arg5) < 0) {
+ goto cleanup;
+ }
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_S390:
+ if (virTypedParamsAddInt(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_INFO_TYPE,
+ VIR_DOMAIN_STATE_INFO_TYPE_QEMU_S390) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddInt(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE,
+ info->data.s390.core) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddULLong(&par, &npar, &maxparams,
+
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK,
+ info->data.s390.psw_mask) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddULLong(&par, &npar, &maxparams,
+
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR,
+ info->data.s390.psw_addr) < 0) {
+ goto cleanup;
+ }
+ if (virTypedParamsAddString(&par, &npar, &maxparams,
+
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON,
+ info->data.s390.reason) < 0) {
+ goto cleanup;
+ }
+ break;
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_NONE:
+ case QEMU_DOMAIN_STATE_PANIC_INFO_TYPE_LAST:
+ if (virTypedParamsAddInt(&par, &npar, &maxparams,
+ VIR_DOMAIN_STATE_PARAMS_INFO_TYPE,
+ VIR_DOMAIN_STATE_INFO_TYPE_NONE) < 0) {
+ goto cleanup;
+ }
+ break;
+ }
+ }
+
+ 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,
@@ -22454,6 +22571,7 @@ static virHypervisorDriver qemuHypervisorDriver = {
.domainGetBlkioParameters = qemuDomainGetBlkioParameters, /* 0.9.0 */
.domainGetInfo = qemuDomainGetInfo, /* 0.2.0 */
.domainGetState = qemuDomainGetState, /* 0.9.2 */
+ .domainGetStateParams = qemuDomainGetStateParams, /* TODO: 5.2.0 */
.domainGetControlInfo = qemuDomainGetControlInfo, /* 0.9.3 */
.domainSave = qemuDomainSave, /* 0.2.0 */
.domainSaveFlags = qemuDomainSaveFlags, /* 0.9.4 */
--
2.17.0
3:04 a.m.
New subject: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available
Add a new parameter to virsh domstate, --info, to report additional
information for the domain state, e.g. for a QEMU guest running a S390
domain:
virsh # domstate --info guest-1
crashed (panicked: s390: core='0' psw-mask='0x0002000180000000' \
psw-addr='0x000000000010f146' reason='disabled-wait')
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>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
tools/virsh-domain-monitor.c | 102 +++++++++++++++++++++++++++++++----
tools/virsh.pod | 5 +-
2 files changed, 95 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index ad739a9d..bf9d4970 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1391,35 +1391,117 @@ 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}
};
+static char *
+vshStateInfoMsgFormat(virTypedParameterPtr params,
+ int nparams)
+{
+ char *ret = NULL;
+ int type;
+
+ if (virTypedParamsGetInt(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_INFO_TYPE, &type) < 0) {
+ return NULL;
+ }
+
+ switch (type) {
+ case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_HYPERV: {
+ unsigned long long arg1, arg2, arg3, arg4, arg5;
+
+ if (virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG1,
&arg1) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG2,
&arg2) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG3,
&arg3) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG4,
&arg4) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG5,
&arg5) < 0) {
+ return NULL;
+ }
+
+ ignore_value(virAsprintf(&ret, "hyper-v: arg1='0x%llx',
arg2='0x%llx', "
+ "arg3='0x%llx', arg4='0x%llx',
arg5='0x%llx'",
+ arg1, arg2, arg3, arg4, arg5));
+ }
+ break;
+ case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_S390: {
+ int core;
+ unsigned long long psw_mask;
+ unsigned long long psw_addr;
+ const char *reason;
+
+ if (virTypedParamsGetInt(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE, &core)
< 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK,
&psw_mask) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR,
&psw_addr) < 0 ||
+ virTypedParamsGetString(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON,
&reason) < 0) {
+ return NULL;
+ }
+
+ ignore_value(virAsprintf(&ret, "s390: core='%d'
psw-mask='0x%016llx' "
+ "psw-addr='0x%016llx'
reason='%s'",
+ core, psw_mask, psw_addr, reason));
+ }
+ break;
+ case VIR_DOMAIN_STATE_INFO_TYPE_NONE:
+ case VIR_DOMAIN_STATE_INFO_TYPE_LAST:
+ break;
+ }
+
+ return ret;
+}
+
static bool
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;
+ 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, &state, &reason, ¶ms, &nparams,
0) < 0) {
+ if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
+ ret = false;
+ goto cleanup;
+ }
}
- if (showReason) {
- vshPrint(ctl, "%s (%s)\n",
- virshDomainStateToString(state),
- virshDomainStateReasonToString(state, reason));
- } else {
- vshPrint(ctl, "%s\n",
- virshDomainStateToString(state));
+ vshPrint(ctl, "%s", virshDomainStateToString(state));
+
+ if (showReason || showInfo) {
+ vshPrint(ctl, " (%s", virshDomainStateReasonToString(state, reason));
+
+ if (showInfo) {
+ info = vshStateInfoMsgFormat(params, nparams);
+ if (info)
+ vshPrint(ctl, ": %s", info);
+ }
+ vshPrint(ctl, ")");
}
+ vshPrint(ctl, "\n");
+
cleanup:
+ VIR_FREE(info);
+ virTypedParamsFree(params, nparams);
virshDomainFree(dom);
return ret;
}
diff --git a/tools/virsh.pod b/tools/virsh.pod
index db723431..82ab83a3 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1568,10 +1568,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
8:05 a.m.
New subject: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available
On 3/25/19 9:04 AM, Bjoern Walk wrote:
Add a new parameter to virsh domstate, --info, to report additional
information for the domain state, e.g. for a QEMU guest running a S390
domain:
virsh # domstate --info guest-1
crashed (panicked: s390: core='0' psw-mask='0x0002000180000000' \
psw-addr='0x000000000010f146' reason='disabled-wait')
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>
Signed-off-by: Bjoern Walk <bwalk(a)linux.ibm.com>
---
tools/virsh-domain-monitor.c | 102 +++++++++++++++++++++++++++++++----
tools/virsh.pod | 5 +-
2 files changed, 95 insertions(+), 12 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index ad739a9d..bf9d4970 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -1391,35 +1391,117 @@ 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}
};
+static char *
+vshStateInfoMsgFormat(virTypedParameterPtr params,
+ int nparams)
+{
+ char *ret = NULL;
+ int type;
+
+ if (virTypedParamsGetInt(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_INFO_TYPE, &type) < 0) {
+ return NULL;
+ }
+
+ switch (type) {
+ case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_HYPERV: {
+ unsigned long long arg1, arg2, arg3, arg4, arg5;
+
+ if (virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG1,
&arg1) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG2,
&arg2) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG3,
&arg3) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG4,
&arg4) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_HYPERV_ARG5,
&arg5) < 0) {
+ return NULL;
+ }
+
+ ignore_value(virAsprintf(&ret, "hyper-v: arg1='0x%llx',
arg2='0x%llx', "
+ "arg3='0x%llx', arg4='0x%llx',
arg5='0x%llx'",
+ arg1, arg2, arg3, arg4, arg5));
+ }
+ break;
+ case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_S390: {
+ int core;
+ unsigned long long psw_mask;
+ unsigned long long psw_addr;
+ const char *reason;
+
+ if (virTypedParamsGetInt(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE,
&core) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK,
&psw_mask) < 0 ||
+ virTypedParamsGetULLong(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR,
&psw_addr) < 0 ||
+ virTypedParamsGetString(params, nparams,
+ VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON,
&reason) < 0) {
+ return NULL;
+ }
+
+ ignore_value(virAsprintf(&ret, "s390: core='%d'
psw-mask='0x%016llx' "
+ "psw-addr='0x%016llx'
reason='%s'",
+ core, psw_mask, psw_addr, reason));
Don't we want to print this in some more machine friendly way? For instance one
argument per line? Also, we have a function (sort of) that prints out typed params. You
may take the inspiration from virshDomainStatsPrintRecord()
+ }
+ break;
+ case VIR_DOMAIN_STATE_INFO_TYPE_NONE:
+ case VIR_DOMAIN_STATE_INFO_TYPE_LAST:
+ break;
+ }
+
+ return ret;
+}
+
static bool
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;
+ 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, &state, &reason, ¶ms, &nparams,
0) < 0) {
+ if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
+ ret = false;
+ goto cleanup;
+ }
}
This looks fishy. Firstly, if --info was requested but we're talking to
an older daemon then virDomainGetStateParams() will fail (the API is not
there) and virshDomainState() is called instead which doesn't fill out
@params and thus we won't print any additional info even though it was
requested.
Also, printing out some error message would be nice:
diff --git i/tools/virsh-domain-monitor.c w/tools/virsh-domain-monitor.c
index bf9d4970a7..27c33f354a 100644
--- i/tools/virsh-domain-monitor.c
+++ w/tools/virsh-domain-monitor.c
@@ -1477,11 +1477,13 @@ cmdDomstate(vshControl *ctl, const vshCmd *cmd)
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
- if (virDomainGetStateParams(dom, &state, &reason, ¶ms, &nparams,
0) < 0) {
- if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
- ret = false;
- goto cleanup;
- }
+ if ((showInfo &&
+ virDomainGetStateParams(dom, &state, &reason, ¶ms, &nparams,
0) < 0) ||
+ (!showInfo &&
+ (state = virshDomainState(ctl, dom, &reason)) < 0)) {
+ vshError(ctl, _("Unable to get state of domain %s"),
virDomainGetName(dom));
+ ret = false;
+ goto cleanup;
}
- if (showReason) {
- vshPrint(ctl, "%s (%s)\n",
- virshDomainStateToString(state),
- virshDomainStateReasonToString(state, reason));
- } else {
- vshPrint(ctl, "%s\n",
- virshDomainStateToString(state));
+ vshPrint(ctl, "%s", virshDomainStateToString(state));
+
+ if (showReason || showInfo) {
+ vshPrint(ctl, " (%s", virshDomainStateReasonToString(state, reason));
+
+ if (showInfo) {
+ info = vshStateInfoMsgFormat(params, nparams);
+ if (info)
+ vshPrint(ctl, ": %s", info);
+ }
+ vshPrint(ctl, ")");
}
+ vshPrint(ctl, "\n");
+
cleanup:
+ VIR_FREE(info);
+ virTypedParamsFree(params, nparams);
virshDomainFree(dom);
return ret;
}
diff --git a/tools/virsh.pod b/tools/virsh.pod
index db723431..82ab83a3 100644
--- a/tools/virsh.pod
+++ b/tools/virsh.pod
@@ -1568,10 +1568,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>
Michal
3:17 a.m.
New subject: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available
Michal Privoznik <mprivozn(a)redhat.com> [2019-03-27, 02:05PM +0100]:
On 3/25/19 9:04 AM, Bjoern Walk wrote:
> + case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_S390: {
> + int core;
> + unsigned long long psw_mask;
> + unsigned long long psw_addr;
> + const char *reason;
> +
> + if (virTypedParamsGetInt(params, nparams,
> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE,
&core) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> +
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK, &psw_mask) < 0 ||
> + virTypedParamsGetULLong(params, nparams,
> +
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR, &psw_addr) < 0 ||
> + virTypedParamsGetString(params, nparams,
> +
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON, &reason) < 0) {
> + return NULL;
> + }
> +
> + ignore_value(virAsprintf(&ret, "s390: core='%d'
psw-mask='0x%016llx' "
> + "psw-addr='0x%016llx'
reason='%s'",
> + core, psw_mask, psw_addr, reason));
Don't we want to print this in some more machine friendly way? For
instance one argument per line? Also, we have a function (sort of)
that prints out typed params. You may take the inspiration from
virshDomainStatsPrintRecord()
One argument per line is certainly doable. How to exploit the print
function I am not so sure, because I want to have special formatting for
different parameters (i.e., the ULL values for PSW_(MASK|ADDR) are hex).
I don't see a way to get around this with a generic function.
> + }
> + break;
> + case VIR_DOMAIN_STATE_INFO_TYPE_NONE:
> + case VIR_DOMAIN_STATE_INFO_TYPE_LAST:
> + break;
> + }
> +
> + return ret;
> +}
> +
> static bool
> 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;
> + 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, &state, &reason, ¶ms,
&nparams, 0) < 0) {
> + if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
> + ret = false;
> + goto cleanup;
> + }
> }
This looks fishy. Firstly, if --info was requested but we're talking to
an older daemon then virDomainGetStateParams() will fail (the API is not
there) and virshDomainState() is called instead which doesn't fill out
@params and thus we won't print any additional info even though it was
requested.
I intent to have the new API function superseed the existing one, so I
actually want to call virDomainGetStateParams() unconditionally and only
fall back to virshDomainState() when the new API is not there (so that
hopefully at some distant point in the future, we could deprecate the
old stuff).
Also, printing out some error message would be nice:
If the new API is not available, I wanted to just silently have the old
behaviour. The domain state and reason can always be retrieve (by means
of the old API) only additional information could be missing. I am not
sure if we should error out if this is the case.
diff --git i/tools/virsh-domain-monitor.c
w/tools/virsh-domain-monitor.c
index bf9d4970a7..27c33f354a 100644
--- i/tools/virsh-domain-monitor.c
+++ w/tools/virsh-domain-monitor.c
@@ -1477,11 +1477,13 @@ cmdDomstate(vshControl *ctl, const vshCmd *cmd)
if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
return false;
- if (virDomainGetStateParams(dom, &state, &reason, ¶ms, &nparams,
0) < 0) {
- if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
- ret = false;
- goto cleanup;
- }
+ if ((showInfo &&
+ virDomainGetStateParams(dom, &state, &reason, ¶ms,
&nparams, 0) < 0) ||
+ (!showInfo &&
+ (state = virshDomainState(ctl, dom, &reason)) < 0)) {
+ vshError(ctl, _("Unable to get state of domain %s"),
virDomainGetName(dom));
+ ret = false;
+ goto cleanup;
}
--
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: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
3:49 a.m.
New subject: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available
On 4/1/19 10:17 AM, Bjoern Walk wrote:
Michal Privoznik <mprivozn(a)redhat.com> [2019-03-27, 02:05PM
+0100]:
> On 3/25/19 9:04 AM, Bjoern Walk wrote:
>> + case VIR_DOMAIN_STATE_INFO_TYPE_QEMU_S390: {
>> + int core;
>> + unsigned long long psw_mask;
>> + unsigned long long psw_addr;
>> + const char *reason;
>> +
>> + if (virTypedParamsGetInt(params, nparams,
>> + VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_CORE,
&core) < 0 ||
>> + virTypedParamsGetULLong(params, nparams,
>> +
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_MASK, &psw_mask) < 0 ||
>> + virTypedParamsGetULLong(params, nparams,
>> +
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_ADDR, &psw_addr) < 0 ||
>> + virTypedParamsGetString(params, nparams,
>> +
VIR_DOMAIN_STATE_PARAMS_PANIC_INFO_S390_PSW_REASON, &reason) < 0) {
>> + return NULL;
>> + }
>> +
>> + ignore_value(virAsprintf(&ret, "s390: core='%d'
psw-mask='0x%016llx' "
>> + "psw-addr='0x%016llx'
reason='%s'",
>> + core, psw_mask, psw_addr, reason));
>
> Don't we want to print this in some more machine friendly way? For
> instance one argument per line? Also, we have a function (sort of)
> that prints out typed params. You may take the inspiration from
> virshDomainStatsPrintRecord()
One argument per line is certainly doable. How to exploit the print
function I am not so sure, because I want to have special formatting for
different parameters (i.e., the ULL values for PSW_(MASK|ADDR) are hex).
I don't see a way to get around this with a generic function.
Ah, okay. Just use whatever you want then. I did not realize this when I
was making the suggestion.
>> + }
>> + break;
>> + case VIR_DOMAIN_STATE_INFO_TYPE_NONE:
>> + case VIR_DOMAIN_STATE_INFO_TYPE_LAST:
>> + break;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static bool
>> 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;
>> + 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, &state, &reason, ¶ms,
&nparams, 0) < 0) {
>> + if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
>> + ret = false;
>> + goto cleanup;
>> + }
>> }
>
> This looks fishy. Firstly, if --info was requested but we're talking to
> an older daemon then virDomainGetStateParams() will fail (the API is not
> there) and virshDomainState() is called instead which doesn't fill out
> @params and thus we won't print any additional info even though it was
> requested.
I intent to have the new API function superseed the existing one, so I
actually want to call virDomainGetStateParams() unconditionally and only
fall back to virshDomainState() when the new API is not there (so that
hopefully at some distant point in the future, we could deprecate the
old stuff).
I understand that but consider the following scenario: a shell script
that calls 'virsh domstate $dom' every so often. If we switch to new API
then this will still fetch expected info for the client but also it will
produce an error message logged at the server side (if it is old and
doesn't support the new API).
That's the main reason I think we should use new API only if necessary,
i.e. if --info was requested. But if you still want to prefer the new
API we should do it the proper way. That is something among these lines:
if (virDomainGetStateParams() < 0) {
if (!last_error || last_error->code != VIR_ERR_NO_SUPPORT)
goto error;
/* fallback */
if (virshDomainState() < 0)
goto error;
}
Note that we have to check for the reason why vurDomainGetStateParams()
failed and fallback on old API if and only if the new API is not supported.
> Also, printing out some error message would be nice:
If the new API is not available, I wanted to just silently have the old
behaviour. The domain state and reason can always be retrieve (by means
of the old API) only additional information could be missing. I am not
sure if we should error out if this is the case.
I think we should. I mean, if I'd run 'virsh domstate --info' that means
I am expecting some extended info on domain state. If I don't get it,
isn't that a failure?
> diff --git i/tools/virsh-domain-monitor.c w/tools/virsh-domain-monitor.c
> index bf9d4970a7..27c33f354a 100644
> --- i/tools/virsh-domain-monitor.c
> +++ w/tools/virsh-domain-monitor.c
> @@ -1477,11 +1477,13 @@ cmdDomstate(vshControl *ctl, const vshCmd *cmd)
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> return false;
>
> - if (virDomainGetStateParams(dom, &state, &reason, ¶ms,
&nparams, 0) < 0) {
> - if ((state = virshDomainState(ctl, dom, &reason)) < 0) {
> - ret = false;
> - goto cleanup;
> - }
> + if ((showInfo &&
> + virDomainGetStateParams(dom, &state, &reason, ¶ms,
&nparams, 0) < 0) ||
> + (!showInfo &&
> + (state = virshDomainState(ctl, dom, &reason)) < 0)) {
> + vshError(ctl, _("Unable to get state of domain %s"),
virDomainGetName(dom));
> + ret = false;
> + goto cleanup;
> }
Michal
6:14 a.m.
New subject: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available
Michal Privoznik <mprivozn(a)redhat.com> [2019-04-01, 10:49AM +0200]:
On 4/1/19 10:17 AM, Bjoern Walk wrote:
> I intent to have the new API function superseed the existing one, so I
> actually want to call virDomainGetStateParams() unconditionally and only
> fall back to virshDomainState() when the new API is not there (so that
> hopefully at some distant point in the future, we could deprecate the
> old stuff).
I understand that but consider the following scenario: a shell script that
calls 'virsh domstate $dom' every so often. If we switch to new API then
this will still fetch expected info for the client but also it will produce
an error message logged at the server side (if it is old and doesn't support
the new API).
That's the main reason I think we should use new API only if necessary, i.e.
if --info was requested. But if you still want to prefer the new API we
should do it the proper way. That is something among these lines:
if (virDomainGetStateParams() < 0) {
if (!last_error || last_error->code != VIR_ERR_NO_SUPPORT)
goto error;
/* fallback */
if (virshDomainState() < 0)
goto error;
}
Note that we have to check for the reason why vurDomainGetStateParams()
failed and fallback on old API if and only if the new API is not supported.
Which would still generate the server-side error message, right? If this
is an issue, it's an issue and I will use your initial suggestion.
Otherwise, something like the above seems cleaner. I will have to think
about this.
> > Also, printing out some error message would be nice:
>
> If the new API is not available, I wanted to just silently have the old
> behaviour. The domain state and reason can always be retrieve (by means
> of the old API) only additional information could be missing. I am not
> sure if we should error out if this is the case.
I think we should. I mean, if I'd run 'virsh domstate --info' that means I
am expecting some extended info on domain state. If I don't get it, isn't
that a failure?
There can also be no additional information available, think different
domain states or other hypervisors, for which also silently no further
information is printed. Should we do something about this scenario too?
7:08 a.m.
New subject: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available
On 4/1/19 1:14 PM, Bjoern Walk wrote:
Michal Privoznik <mprivozn(a)redhat.com> [2019-04-01, 10:49AM
+0200]:
> On 4/1/19 10:17 AM, Bjoern Walk wrote:
>> I intent to have the new API function superseed the existing one, so I
>> actually want to call virDomainGetStateParams() unconditionally and only
>> fall back to virshDomainState() when the new API is not there (so that
>> hopefully at some distant point in the future, we could deprecate the
>> old stuff).
>
> I understand that but consider the following scenario: a shell script that
> calls 'virsh domstate $dom' every so often. If we switch to new API then
> this will still fetch expected info for the client but also it will produce
> an error message logged at the server side (if it is old and doesn't support
> the new API).
>
> That's the main reason I think we should use new API only if necessary, i.e.
> if --info was requested. But if you still want to prefer the new API we
> should do it the proper way. That is something among these lines:
>
> if (virDomainGetStateParams() < 0) {
> if (!last_error || last_error->code != VIR_ERR_NO_SUPPORT)
> goto error;
>
> /* fallback */
> if (virshDomainState() < 0)
> goto error;
> }
>
> Note that we have to check for the reason why vurDomainGetStateParams()
> failed and fallback on old API if and only if the new API is not supported.
Which would still generate the server-side error message, right?
Yes, it would.
If this
is an issue, it's an issue and I will use your initial suggestion.
Otherwise, something like the above seems cleaner. I will have to think
about this.
Well, to be fair we use something like this. For instance when listing
domains/networks/... But there is a clear advantage of using 'new'
virConnectListAll*() over 'old' GetNumDomains() + ListDomains() +
GetDefinedDomains() + ListDefinedDomains() - they are atomic. But what
is the advantage of using the new API in case of plain 'virsh domstate'?
BTW: We don't really have notion of deprecating an API. We can merely
say 'don't use this, use that because reasons' in the docs. But we still
have to fix all APIs / maitain them.
This is the reason I am advocating for using new API only if neccessary.
>>> Also, printing out some error message would be nice:
>>
>> If the new API is not available, I wanted to just silently have the old
>> behaviour. The domain state and reason can always be retrieve (by means
>> of the old API) only additional information could be missing. I am not
>> sure if we should error out if this is the case.
>
> I think we should. I mean, if I'd run 'virsh domstate --info' that means
I
> am expecting some extended info on domain state. If I don't get it, isn't
> that a failure?
There can also be no additional information available, think different
domain states or other hypervisors, for which also silently no further
information is printed. Should we do something about this scenario too?
No. That is perfectly okay. The API succeeded, but there is no
additional info.
Michal
8:57 a.m.
New subject: [libvirt] [PATCH v2 6/7] virsh: domstate: report detailed state if available
Michal Privoznik <mprivozn(a)redhat.com> [2019-04-01, 02:08PM +0200]:
On 4/1/19 1:14 PM, Bjoern Walk wrote:
> Michal Privoznik <mprivozn(a)redhat.com> [2019-04-01, 10:49AM +0200]:
> > On 4/1/19 10:17 AM, Bjoern Walk wrote:
> > > I intent to have the new API function superseed the existing one, so I
> > > actually want to call virDomainGetStateParams() unconditionally and only
> > > fall back to virshDomainState() when the new API is not there (so that
> > > hopefully at some distant point in the future, we could deprecate the
> > > old stuff).
> >
> > I understand that but consider the following scenario: a shell script that
> > calls 'virsh domstate $dom' every so often. If we switch to new API
then
> > this will still fetch expected info for the client but also it will produce
> > an error message logged at the server side (if it is old and doesn't
support
> > the new API).
> >
> > That's the main reason I think we should use new API only if necessary,
i.e.
> > if --info was requested. But if you still want to prefer the new API we
> > should do it the proper way. That is something among these lines:
> >
> > if (virDomainGetStateParams() < 0) {
> > if (!last_error || last_error->code != VIR_ERR_NO_SUPPORT)
> > goto error;
> >
> > /* fallback */
> > if (virshDomainState() < 0)
> > goto error;
> > }
> >
> > Note that we have to check for the reason why vurDomainGetStateParams()
> > failed and fallback on old API if and only if the new API is not supported.
>
> Which would still generate the server-side error message, right?
Yes, it would.
> If this
> is an issue, it's an issue and I will use your initial suggestion.
> Otherwise, something like the above seems cleaner. I will have to think
> about this.
Well, to be fair we use something like this. For instance when listing
domains/networks/... But there is a clear advantage of using 'new'
virConnectListAll*() over 'old' GetNumDomains() + ListDomains() +
GetDefinedDomains() + ListDefinedDomains() - they are atomic. But what is
the advantage of using the new API in case of plain 'virsh domstate'?
BTW: We don't really have notion of deprecating an API. We can merely say
'don't use this, use that because reasons' in the docs. But we still have to
fix all APIs / maitain them.
This is the reason I am advocating for using new API only if neccessary.
Ok, I am convinced now :)
Thanks for your advice, I will send v3 after the release.
3:04 a.m.
New subject: [libvirt] [PATCH v2 7/7] news: add entry for virDomainGetStateParams
Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
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 5c3028e1..09617d6a 100644
--- a/docs/news.xml
+++ b/docs/news.xml
@@ -35,6 +35,17 @@
<libvirt>
<release version="v5.2.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>
Add Storage Pool Capabilities output
--
2.17.0
12:07 a.m.
Bjoern Walk <bwalk(a)linux.ibm.com> [2019-03-25, 09:04AM +0100]:
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 guest1
crashed (panicked: s390: core='1' psw-mask='0x1234000000000000' \
psw-addr='0x0000000000001234' reason='disabled-wait')
Any chance we might squeeze this one in before the freeze?
8:05 a.m.
On 3/25/19 9:04 AM, 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 guest1
crashed (panicked: s390: core='1' psw-mask='0x1234000000000000' \
psw-addr='0x0000000000001234' reason='disabled-wait')
Previous version is here:
https://www.redhat.com/archives/libvir-list/2018-July/msg00686.html
v1 -> v2:
* refactored the public API according to discussions to provide a
more machine-parsable interface
Bjoern Walk (7):
qemu: monitor: move event data structure to domain
qemu: domain: store and update panic info
lib: introduce virDomainGetStateParams function
remote: implement remoteDomainGetStateParams
qemu: implement qemuDomainGetStateParams
virsh: domstate: report detailed state if available
news: add entry for virDomainGetStateParams
docs/news.xml | 11 +++
include/libvirt/libvirt-domain.h | 76 +++++++++++++++++
src/driver-hypervisor.h | 9 ++
src/libvirt-domain.c | 64 ++++++++++++++
src/libvirt_public.syms | 1 +
src/qemu/qemu_domain.c | 89 +++++++++++++++++++-
src/qemu/qemu_domain.h | 47 +++++++++++
src/qemu/qemu_driver.c | 126 +++++++++++++++++++++++++++-
src/qemu/qemu_monitor.c | 53 +-----------
src/qemu/qemu_monitor.h | 48 ++---------
src/qemu/qemu_monitor_json.c | 20 ++---
src/qemu/qemu_process.c | 2 +-
src/remote/remote_daemon_dispatch.c | 50 +++++++++++
src/remote/remote_driver.c | 44 ++++++++++
src/remote/remote_protocol.x | 22 ++++-
src/remote_protocol-structs | 12 +++
tools/virsh-domain-monitor.c | 102 +++++++++++++++++++---
tools/virsh.pod | 5 +-
18 files changed, 658 insertions(+), 123 deletions(-)
I like the general idea, but some cleanups are needed IMO.
Michal
2092
days inactive
2099
days old
21 comments
2 participants
participants (2)
-
Bjoern Walk -
Michal Privoznik