[libvirt] [PATCH 39/41] cpu: Rework cpuCompare* APIs

[Jiri Denemark]

On Tue, Aug 30, 2016 at 17:57:01 -0400, John Ferlan wrote:
...
> > - * cpuCompareXML:
> > + * virCPUCompareXML:
> >   *
> > + * @arch: CPU architecture
> >   * @host: host CPU definition
> >   * @xml: XML description of either guest or host CPU to be compared with @host
> 
> Existing, @failIncompatible description is missing

I added a separate patch to fix this.

> >   *
> > @@ -104,25 +105,26 @@ cpuGetSubDriverByName(const char *name)
> >   * the @host CPU.
> >   */
> >  virCPUCompareResult
> > -cpuCompareXML(virCPUDefPtr host,
> > -              const char *xml,
> > -              bool failIncompatible)
> > +virCPUCompareXML(virArch arch,
> > +                 virCPUDefPtr host,
> > +                 const char *xml,
> > +                 bool failIncompatible)
> >  {
> >      xmlDocPtr doc = NULL;
> >      xmlXPathContextPtr ctxt = NULL;
> >      virCPUDefPtr cpu = NULL;
> >      virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR;
> >  
> > -    VIR_DEBUG("host=%p, xml=%s", host, NULLSTR(xml));
> > +    VIR_DEBUG("arch=%s, host=%p, xml=%s",
> > +              virArchToString(arch), host, NULLSTR(xml));
> 
> The prototype changed such that 'host' and 'xml' could be passed as NULL
> without a compiler complaint (ok a static analysis complaint).  Was that
> by design?

host == NULL is definitely allowed by design, since we want to be able
to successfully compare CPUs on ARM where we are not able to detect the
host CPU model.

I'm not exactly sure about the xml argument, but since NULLSTR(xml) was
already here I figured this is a good place to return an error for xml
== NULL rather than forcing all callers to do this check. However I
apparently didn't implement the check :-)

> >      if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), &ctxt)))
> 
> Having xml as NULL probably doesn't work well here.

I added an explicit check for xml == NULL.

> >          goto cleanup;
> >  
> > -    cpu = virCPUDefParseXML(ctxt->node, ctxt, VIR_CPU_TYPE_AUTO);
> > -    if (cpu == NULL)
> > +    if (!(cpu = virCPUDefParseXML(ctxt->node, ctxt, VIR_CPU_TYPE_AUTO)))
> >          goto cleanup;
> >  
> > -    ret = cpuCompare(host, cpu, failIncompatible);
> > +    ret = virCPUCompare(arch, host, cpu, failIncompatible);
> 
> Allowing host to be NULL will cause failure in PPC and X86 which perhaps
> is expected.

Yes, although it will partially change in the future.

...
> >  virCPUCompareResult
> > -cpuCompareXML(virCPUDefPtr host,
> > -              const char *xml,
> > -              bool failIncompatible)
> > -    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> 
> Was removing NONNULL by design?  I think it needs to be replaced for xml
> at least.

Yup, it was done by design.

...
> > @@ -1057,7 +1057,8 @@ x86ModelFromCPU(const virCPUDef *cpu,
> >          policy != -1)
> >          return x86ModelNew();
> >  
> > -    if (policy == VIR_CPU_FEATURE_REQUIRE || policy == -1) {
> > +    if (cpu->model &&
> > +        (policy == VIR_CPU_FEATURE_REQUIRE || policy == -1)) {
> 
> Shall I assume this related to the compare patch? or a fix as a result?
> I assume it's related to changes in x86Compute and x86GuestData, but it
> wasn't really clear from the commit message.

Yes, it's related to this patch and required to do what commit message
says:

    "Both cpuCompare* APIs are renamed to virCPUCompare*. And they
    should now work for any guest CPU definition, i.e., even for
    host-passthrough (trivial) and host-model CPUs."

Thanks to this x86ModelFromCPU() may now be called for CPU definitions
without a model, e.g., CPUs with host-model mode.

Jirka