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

[Jiri Denemark]

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