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

12 Oct 2017

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@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@redhat.com>

John

    