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

[John Ferlan]

On 10/13/2017 10:39 AM, Jiri Denemark wrote:
> 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
> 

I think the first comment after ...why was more what I was wondering
(vis-a-vis the usage of a local @name when direct assignment would also
work) ... then I started thinking was there some edge condition that you
were avoiding hence the extra cruft. It wouldn't seem right that you'd
get a NULL blockers entry from QEMU, but then again making that
assumption can be risky too (since VIR_STRDUP(xxx, NULL), just assigns
NULL to xxx and doesn't cause an error. No matter - just me overthinking
and overtyping.


John