[libvirt] [PATCH 04/23] qemu: Parse unavailable features for CPU models

[John Ferlan]

On 10/04/2017 10:58 AM, Jiri Denemark wrote:
> query-cpu-definitions QMP command returns a list of unavailable features
> which prevent CPU models from being usable on the current host. So far
> we only checked whether the list was empty to mark CPU models as
> (un)usable. This patch parses all unavailable features for each CPU
> model and stores them in virDomainCapsCPUModel as a list of usability
> blockers.
> 
> Signed-off-by: Jiri Denemark <jdenemar at redhat.com>
> ---
>  src/qemu/qemu_capabilities.c                      |    2 +-
>  src/qemu/qemu_monitor.c                           |    2 +
>  src/qemu/qemu_monitor.h                           |    1 +
>  src/qemu/qemu_monitor_json.c                      |   26 +-
>  tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml  | 1102 +++++++++++++++++++--
>  tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml |  236 ++++-
>  tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml  |  154 ++-
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml  |  154 ++-
>  8 files changed, 1556 insertions(+), 121 deletions(-)
> 

[...]

> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index c63d250d36..fcdd58b369 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5076,6 +5076,8 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>  
>          if (virJSONValueObjectHasKey(child, "unavailable-features")) {
>              virJSONValuePtr blockers;
> +            size_t j;
> +            int len;
>  
>              blockers = virJSONValueObjectGetArray(child,
>                                                    "unavailable-features");
> @@ -5086,10 +5088,32 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>                  goto cleanup;
>              }
>  
> -            if (virJSONValueArraySize(blockers) > 0)
> +            len = virJSONValueArraySize(blockers);
> +
> +            if (len != 0)

At this point len is either 0 (empty) or > 0 because if it was < 0 the
virJSONValueObjectGetArray would have already caused a failure, right?

So why not :

    if (len == 0) {
        cpu->usable = VIR_TRISTATE_BOOL_YES;
        continue;
    }

    cpu->usable = VIR_TRISTATE_BOOL_NO;
    if (VIR_ALLOC_N(cpu->blockers, len + 1) < 0)
...


>                  cpu->usable = VIR_TRISTATE_BOOL_NO;
>              else
>                  cpu->usable = VIR_TRISTATE_BOOL_YES;
> +
> +            if (len > 0 && VIR_ALLOC_N(cpu->blockers, len + 1) < 0)
> +                goto cleanup;
> +
> +            for (j = 0; j < len; j++) {
> +                virJSONValuePtr blocker = virJSONValueArrayGet(blockers, j);
> +                char *name;

virJSONValueArrayGet can return NULL, but that shouldn't affect us since
blockers is an ARRAY and there's no way j >= len...

> +
> +                if (blocker->type != VIR_JSON_TYPE_STRING) {
> +                    virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                                   _("unexpected value in unavailable-features "
> +                                     "array"));
> +                    goto cleanup;
> +                }

...but because of this check...

> +
> +                if (VIR_STRDUP(name, virJSONValueGetString(blocker)) < 0)
...wouldn't virJSONValueGetString return a non NULL string, so...

> +                    goto cleanup;
> +
> +                cpu->blockers[j] = name;

...why not just go direct to cpu->blockers[j]?  Or did you come across
something in testing that had a NULL return from the call to
virJSONValueGetString(blocker)?

Maybe a NULL entry should just be ignored so as to not tank the whole
thing using the logic that if a blocker isn't there, by name, then is it
a blocker?

> +            }
>          }
>      }
>  

Reviewed-by: John Ferlan <jferlan at redhat.com>

John