[libvirt] [PATCH 07/11] qemu: chardev: Extract more information about character devices

[Jiri Denemark]

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