[libvirt] [PATCH 14/14] cputest: Add tests for virCPUUpdateLive API

[Jiri Denemark]

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 at redhat.com>
> > ---
> >  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