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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list