Re: [libvirt] [PATCH 39/41] cpu: Rework cpuCompare* APIs
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