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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
