[libvirt] [PATCH v2 0/7] extend virsh domstate to show additional information
This patch series introduces the ability to save additional information for the domain state and exposes this information in virsh domstate. For example in the case of QEMU guest panic events, we can provide additional information like the crash reason or register state of the domain. This information usually gets logged in the domain log but for debugging it is useful to have it accessible from the client. Therefore, let's introduce a new public API function, virDomainGetStateParams, an extensible version of virDomainGetState, which returns the complete state of the domain, including newly introduced additional information. Let's also extend virsh domstate and introduce a new parameter --info to show the domain state, reason and additional information when available. virsh domstate --info 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
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@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
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@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
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@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
Michal Privoznik <mprivozn@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
This API function extends the virDomainGetState function by returning additional state information as a dictionary of typed parameters. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
Implement the API function virDomainGetStateParams for the remote driver and wire up the remote protocol. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
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@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
Michal Privoznik <mprivozn@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
On 3/27/19 2:19 PM, Bjoern Walk wrote:
Michal Privoznik <mprivozn@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
Implement the API function virDomainGetStateParams for the QEMU hypervisor driver. Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
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@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
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@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@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
Michal Privoznik <mprivozn@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
On 4/1/19 10:17 AM, Bjoern Walk wrote:
Michal Privoznik <mprivozn@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
Michal Privoznik <mprivozn@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?
On 4/1/19 1:14 PM, Bjoern Walk wrote:
Michal Privoznik <mprivozn@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
Michal Privoznik <mprivozn@redhat.com> [2019-04-01, 02:08PM +0200]:
On 4/1/19 1:14 PM, Bjoern Walk wrote:
Michal Privoznik <mprivozn@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.
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Signed-off-by: Bjoern Walk <bwalk@linux.ibm.com> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index 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
Bjoern Walk <bwalk@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?
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
participants (2)
-
Bjoern Walk -
Michal Privoznik