On Wed, Nov 19, 2014 at 11:23:20 +0100, Peter Krempa wrote:
Improve the monitor function to also retrieve the guest state of character device (if provided) so that we can refresh the state of virtio-serial channels and perhaps react to changes in the state in future patches.
This patch changes the returned data from qemuMonitorGetChardevInfo to return a structure containing the pty path and the state if the info is relevant.
The change to the testsuite makes sure that the data is parsed correctly. --- src/qemu/qemu_monitor.c | 13 +++++++++++- src/qemu/qemu_monitor.h | 6 ++++++ src/qemu/qemu_monitor_json.c | 48 +++++++++++++++++++++++++++++++++----------- src/qemu/qemu_monitor_text.c | 17 +++++++++++----- src/qemu/qemu_process.c | 8 ++++---- tests/qemumonitorjsontest.c | 39 +++++++++++++++++++++++++++++------ 6 files changed, 103 insertions(+), 28 deletions(-)
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 926619f..c9c84f9 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2982,6 +2982,17 @@ qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias, }
+static void +qemuMonitorChardevInfoFree(void *data, + const void *name ATTRIBUTE_UNUSED) +{ + qemuMonitorChardevInfoPtr info = data; + + VIR_FREE(info->ptyPath); + VIR_FREE(info); +} + + int qemuMonitorGetChardevInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo) @@ -2997,7 +3008,7 @@ qemuMonitorGetChardevInfo(qemuMonitorPtr mon, goto error; }
- if (!(info = virHashCreate(10, virHashValueFree))) + if (!(info = virHashCreate(10, qemuMonitorChardevInfoFree))) goto error;
if (mon->json) diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 3078be0..21533a4 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -649,6 +649,12 @@ int qemuMonitorRemoveNetdev(qemuMonitorPtr mon, int qemuMonitorQueryRxFilter(qemuMonitorPtr mon, const char *alias, virNetDevRxFilterPtr *filter);
+typedef struct _qemuMonitorChardevInfo qemuMonitorChardevInfo; +typedef qemuMonitorChardevInfo *qemuMonitorChardevInfoPtr; +struct _qemuMonitorChardevInfo { + char *ptyPath; + virDomainChrDeviceState state; +}; int qemuMonitorGetChardevInfo(qemuMonitorPtr mon, virHashTablePtr *retinfo);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9c8a6fb..5429382 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3426,6 +3426,9 @@ qemuMonitorJSONExtractChardevInfo(virJSONValuePtr reply, virJSONValuePtr data; int ret = -1; size_t i; + char *ptyPath = NULL; + virDomainChrDeviceState state; + qemuMonitorChardevInfoPtr entry = NULL;
if (!(data = virJSONValueObjectGet(reply, "return"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", @@ -3440,44 +3443,65 @@ qemuMonitorJSONExtractChardevInfo(virJSONValuePtr reply, }
for (i = 0; i < virJSONValueArraySize(data); i++) { - virJSONValuePtr entry = virJSONValueArrayGet(data, i); + virJSONValuePtr chardev = virJSONValueArrayGet(data, i); const char *type; - const char *id; - if (!entry) { + const char *alias; + bool connected; + + if (!chardev) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("character device information was missing array element")); goto cleanup; }
- if (!(type = virJSONValueObjectGetString(entry, "filename"))) { + if (!(alias = virJSONValueObjectGetString(chardev, "label"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("character device information was missing filename")); + _("character device information was missing alias"));
Shouldn't we report that device information was missing label rather than alias as we are referring to a field in the QMP event?
goto cleanup; }
- if (!(id = virJSONValueObjectGetString(entry, "label"))) { + if (!(type = virJSONValueObjectGetString(chardev, "filename"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("character device information was missing filename")); goto cleanup; }
- if (STRPREFIX(type, "pty:")) { - char *path; - if (VIR_STRDUP(path, type + strlen("pty:")) < 0) + if (STRPREFIX(type, "pty:") && + VIR_STRDUP(ptyPath, type + strlen("pty:")) < 0) + goto cleanup; + + if (virJSONValueObjectGetBoolean(chardev, "frontend-open", &connected) == 0) { + if (connected) + state = VIR_DOMAIN_CHR_DEVICE_STATE_CONNECTED; + else + state = VIR_DOMAIN_CHR_DEVICE_STATE_DISCONNECTED; + } + + if (ptyPath || state != VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT) {
It took me a while to understand what you want to do here :-) So you want store only "pty:" char devices or char devices with "frontend-open" field, right? Any reason for this filtering (apart from the fact we don't care about other devices now)? I guess storing all devices would work too and the code would be a bit simpler. Or am I wrong?
+ if (VIR_ALLOC(entry) < 0) goto cleanup;
- if (virHashAddEntry(info, id, path) < 0) { + entry->ptyPath = ptyPath; + entry->state = state; + + if (virHashAddEntry(info, alias, entry) < 0) { virReportError(VIR_ERR_OPERATION_FAILED, - _("failed to save chardev path '%s'"), path); - VIR_FREE(path); + _("failed to add chardev '%s' info"), alias); goto cleanup; } + + entry = NULL; + ptyPath = NULL; + state = VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT;
Ah, you managed to confuse me again :-) I'd prefer if you moved the "virDomainChrDeviceState state;" declaration inside the for loop and initialized state to VIR_DOMAIN_CHR_DEVICE_STATE_DEFAULT there. However, if you follow my suggestion to remove the condition above, you can avoid state variable completely and rather assign to entry->state directly (after moving the allocation of entry before checking for frontend-open, of course).
} }
ret = 0;
cleanup: + VIR_FREE(entry); + VIR_FREE(ptyPath); + return ret; }
... Jirka