On Thu, Oct 12, 2017 at 07:25:59 -0400, John Ferlan wrote:
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. [...]
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 @@ -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)
OK
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...
Right.
+ + 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...
Sure, but this is not checking whether virJSONValueGetString returns NULL or not, this checks whether we successfully made a copy of the blocker string.
+ 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?
I'm not really sure what you were trying to say. Is the temporary name variable confusing you? No problem, it's not needed at all, I changed the code so that VIR_STRDUP stores the result directly in cpu->blockers[j]. Jirka