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

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