On Wed, Apr 05, 2017 at 07:37:34 -0400, John Ferlan wrote:
>
>
> On 03/17/2017 12:36 PM, Jiri Denemark wrote:
> > The test takes
> >
> > x86-cpuid-Something-guest.xml CPU (the CPU libvirt would use for
> > host-model on a CPU described by x86_64-cpuid-Something.xml without
> > talking to QEMU about what it supports on the host)
> >
> > and updates it according to CPUID data from QEMU:
> >
> > x86_64-cpuid-Something-enabled.xml (reported as "feature-words"
> > property of the CPU device)
> >
> > and
> >
> > x86_64-cpuid-Something-disabled.xml (reported as "filtered-features"
> > property of the CPU device).
> >
> > The result is compared to
> >
> > x86_64-cpuid-Something-json.xml (the CPU libvirt would use as
> > host-model based on the reply from query-cpu-model-expansion).
> >
> > The comparison is a bit tricky because the *-json.xml CPU contains fewer
> > disabled features. Only the features which are included in the base CPU
> > model, but listed as disabled in *.json will be disabled in *-json.xml.
> > The CPU computed by virCPUUpdateLive from the test data will list all
> > features present in the host's CPUID data and not enabled in *.json as
> > disabled. The cpuTestUpdateLiveCompare function checks that the computed
> > and expected sets of enabled features match.
> >
> > Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> > ---
> > tests/cputest.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 149 insertions(+)
> >
> > diff --git a/tests/cputest.c b/tests/cputest.c
> > index 4a4d427e1..2c64c2cd0 100644
> > --- a/tests/cputest.c
> > +++ b/tests/cputest.c
> > @@ -523,6 +523,151 @@ cpuTestGuestCPUID(const void *arg)
> > }
> >
> >
> > +static int
> > +cpuTestUpdateLiveCompare(virArch arch,
> > + virCPUDefPtr actual,
> > + virCPUDefPtr expected)
> > +{
> > + size_t i, j;
> > + int ret = 0;
> > +
> > + if (virCPUExpandFeatures(arch, actual) < 0 ||
> > + virCPUExpandFeatures(arch, expected) < 0)
> > + return -1;
> > +
> > + if (STRNEQ(actual->model, expected->model)) {
> > + VIR_TEST_VERBOSE("Actual CPU model '%s', expected '%s'\n",
> > + actual->model, expected->model);
> > + return -1;
> > + }
> > +
> > + i = j = 0;
> > + while (i < actual->nfeatures || j < expected->nfeatures) {
> > + virCPUFeatureDefPtr featAct = NULL;
> > + virCPUFeatureDefPtr featExp = NULL;
> > + int cmp;
> > +
> > + if (i < actual->nfeatures)
> > + featAct = actual->features + i;
> > +
> > + if (j < expected->nfeatures)
> > + featExp = expected->features + j;
> > +
> > + /*
> > + * Act < Exp => cmp < 0 (missing entry in Exp)
> > + * Act = Exp => cmp = 0
> > + * Act > Exp => cmp > 0 (missing entry in Act)
> > + *
> > + * NULL > name for any name != NULL
> > + */
> > + if (featAct && featExp)
> > + cmp = strcmp(featAct->name, featExp->name);
> > + else
> > + cmp = featExp ? 1 : -1;
> > +
> > + if (cmp <= 0)
> > + i++;
> > + if (cmp >= 0)
> > + j++;
> > +
> > + /* Possible combinations of cmp, featAct->policy, and featExp->policy:
> > + * cmp Act Exp result
> > + * ---------------------------------
> > + * 0 dis dis ok
> > + * 0 dis req missing
> > + * 0 req dis extra
> > + * 0 req req ok
> > + * ---------------------------------
> > + * - dis X ok # ignoring extra disabled features
> > + * - req X extra
> > + * ---------------------------------
> > + * + X dis extra
> > + * + X req missing
> > + */
> > + if ((cmp == 0 &&
> > + featAct->policy == VIR_CPU_FEATURE_DISABLE &&
> > + featExp->policy == VIR_CPU_FEATURE_REQUIRE) ||
> > + (cmp > 0 &&
> > + featExp->policy == VIR_CPU_FEATURE_REQUIRE)) {
> > + VIR_TEST_VERBOSE("Actual CPU lacks feature '%s'\n",
> > + featExp->name);
> > + ret = -1;
> > + continue;
> > + }
> > +
> > + if ((cmp == 0 &&
> > + featAct->policy == VIR_CPU_FEATURE_REQUIRE &&
> > + featExp->policy == VIR_CPU_FEATURE_DISABLE) ||
> > + (cmp < 0 &&
> > + featAct->policy == VIR_CPU_FEATURE_REQUIRE) ||
> > + (cmp > 0 &&
> > + featExp->policy == VIR_CPU_FEATURE_DISABLE)) {
> > + VIR_TEST_VERBOSE("Actual CPU has extra feature '%s'\n",
> > + featAct->name);
>
> I know it's only a test, but this log message raised Coverity's
> attention because it's not sure featAct is NULL at this point if "cmp >
> 0". So that I can at least mask it my local tree - could featAct be
> NULL or should there be a different message if cmp > 0?
Oops, featAct should be used only if cmp <= 0, otherwise it will either
be NULL or irrelevant and featExp should be used instead.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
