On Sun, Dec 18, 2016 at 14:22:27 -0500, Jason J. Herne wrote:
> When qmp query-cpu-model-expansion is available probe Qemu for its view of the
> host model. In kvm environments this can provide a more complete view of the
> host model because features supported by Qemu and Kvm can be considered.
>
> Signed-off-by: Collin L. Walling <walling@xxxxxxxxxxxxxxxxxx>
> Signed-off-by: Jason J. Herne <jjherne@xxxxxxxxxxxxxxxxxx>
>
> # Conflicts:
> # tests/qemucapabilitiesdata/caps_2.8.0.s390x.replies
>
> Signed-off-by: Jason J. Herne <jjherne@xxxxxxxxxxxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 185 ++++++++++++++++++++-
> tests/domaincapsschemadata/qemu_2.7.0.s390x.xml | 4 +-
> tests/domaincapsschemadata/qemu_2.8.0.s390x.xml | 17 +-
> .../qemucapabilitiesdata/caps_2.8.0.s390x.replies | 26 +++
> tests/qemucapabilitiesdata/caps_2.8.0.s390x.xml | 17 ++
> 5 files changed, 239 insertions(+), 10 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index bff30ed..7d33b19 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
...
> @@ -3055,6 +3117,98 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
> virResetLastError();
> }
>
One more empty line here, please.
> +void
> +virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
> + virCapsPtr caps)
> +{
> + if (!caps || !virQEMUCapsGuestIsNative(caps->host.arch, qemuCaps->arch))
> + return;
> +
> + switch (qemuCaps->arch) {
> + case VIR_ARCH_S390:
> + case VIR_ARCH_S390X:
> + virQEMUCapsCopyCPUModelFromQEMU(qemuCaps);
> + break;
> + default: virQEMUCapsCopyCPUModelFromHost(qemuCaps, caps);
default:
virQEMUCapsCopyCPUModelFromHost(qemuCaps, caps);
> + }
> +}
> +
> +
> +static int
> +virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> + xmlXPathContextPtr ctxt)
> +{
> + char *str = NULL;
> + xmlNodePtr hostCPUNode;
> + xmlNodePtr *featureNodes = NULL;
> + xmlNodePtr oldnode = ctxt->node;
> + qemuMonitorCPUModelInfoPtr hostCPU = NULL;
> + int ret = -1;
> + size_t i;
> + int n;
> +
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) {
> + ret = 0;
> + goto cleanup;
> + }
This is not really necessary, properly checking for <hostCPU> element is
enough.
> +
> + if (!(hostCPUNode = virXPathNode("./hostCPU", ctxt)) ||
> + VIR_ALLOC(hostCPU) < 0)
> + goto cleanup;
When QEMU is probed in TCG mode, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION
might be set but ./hostCPU will not be present in the capabilities XML.
The code should not fail in such a case. In other words:
if (!(hostCPUNode = virXPathNode("./hostCPU", ctxt))) {
ret = 0;
goto cleanup;
}
if (VIR_ALLOC(hostCPU) < 0)
goto cleanup;
> +
> + if ((str = virXMLPropString(hostCPUNode, "model"))) {
Don't confuse virXMLPropString and virXPathString. The latter does not
make a copy of the string while the former does make the copy.
> + if (VIR_STRDUP(hostCPU->name, str) < 0)
> + goto cleanup;
> + }
Anyway, it looks like the name attribute should always be present, so
if (!(hostCPU->name = virXMLPropString(hostCPUNode, "model"))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing host CPU model name in QEMU "
"capabilities cache"));
goto cleanup;
}
should be enough.
> +
> + ctxt->node = hostCPUNode;
> +
> + if ((n = virXPathNodeSet("./feature", ctxt, &featureNodes)) > 0) {
> + if (VIR_ALLOC_N(hostCPU->props, n) < 0)
> + goto cleanup;
> +
> + hostCPU->nprops = n;
> +
> + for (i = 0; i < n; i++) {
> + if (!(str = virXMLPropString(featureNodes[i], "name"))) {
You can directly assign it to hostCPU->props[i].name.
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing 'name' element for a host CPU model"
'name' is not an element, it's an attribute.
> + " feature in QEMU capabilities cache"));
> + goto cleanup;
> + }
> + if (VIR_STRDUP(hostCPU->props[i].name, str) < 0)
> + goto cleanup;
Let's separate the two attributes by an empty line here.
> + if (!(str = virXMLPropString(featureNodes[i], "supported"))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("missing 'supported' element for a host CPU"
s/element/attribute/
> + " model feature in QEMU capabilities cache"));
> + goto cleanup;
> + }
> + if (STREQ(str, "yes")) {
> + hostCPU->props[i].supported = true;
> + } else if (STREQ(str, "no")) {
> + hostCPU->props[i].supported = false;
> + } else {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("malformed supported value: expected 'yes'"
> + " or 'no'"));
It's an internal XML, not something a user is supposed to create so
printing the incorrect value should be enough:
..., _("invalid supported value: '%s'"), str);
> + goto cleanup;
> + }
You'd leak str here if n > 1.
> + }
> + }
> +
> + qemuCaps->hostCPUModelInfo = hostCPU;
> + hostCPU = NULL;
> + ret = 0;
> +
> + cleanup:
> + ctxt->node = oldnode;
> + VIR_FREE(str);
> + VIR_FREE(featureNodes);
> + qemuMonitorCPUModelInfoFree(hostCPU);
> + return ret;
> +}
> +
>
> static int
> virQEMUCapsLoadCPUModels(virQEMUCapsPtr qemuCaps,
...
> diff --git a/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml b/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml
> index 9f181d3..999e279 100644
> --- a/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml
> +++ b/tests/domaincapsschemadata/qemu_2.7.0.s390x.xml
> @@ -20,9 +20,7 @@
> </os>
> <cpu>
> <mode name='host-passthrough' supported='yes'/>
> - <mode name='host-model' supported='yes'>
> - <model fallback='allow'></model>
> - </mode>
> + <mode name='host-model' supported='no'/>
> <mode name='custom' supported='no'/>
> </cpu>
> <devices>
This hunk should be part of 4/9.
ACK with the attached diff squashed in...
Jirka
diff --git i/src/qemu/qemu_capabilities.c w/src/qemu/qemu_capabilities.c
index a3f97d209..2512e4838 100644
--- i/src/qemu/qemu_capabilities.c
+++ w/src/qemu/qemu_capabilities.c
@@ -3117,6 +3117,7 @@ virQEMUCapsCopyCPUModelFromHost(virQEMUCapsPtr qemuCaps,
virResetLastError();
}
+
void
virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
virCapsPtr caps)
@@ -3129,7 +3130,9 @@ virQEMUCapsInitHostCPUModel(virQEMUCapsPtr qemuCaps,
case VIR_ARCH_S390X:
virQEMUCapsCopyCPUModelFromQEMU(qemuCaps);
break;
- default: virQEMUCapsCopyCPUModelFromHost(qemuCaps, caps);
+
+ default:
+ virQEMUCapsCopyCPUModelFromHost(qemuCaps, caps);
}
}
@@ -3147,18 +3150,19 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
size_t i;
int n;
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION)) {
+ if (!(hostCPUNode = virXPathNode("./hostCPU", ctxt))) {
ret = 0;
goto cleanup;
}
- if (!(hostCPUNode = virXPathNode("./hostCPU", ctxt)) ||
- VIR_ALLOC(hostCPU) < 0)
+ if (VIR_ALLOC(hostCPU) < 0)
goto cleanup;
- if ((str = virXMLPropString(hostCPUNode, "model"))) {
- if (VIR_STRDUP(hostCPU->name, str) < 0)
- goto cleanup;
+ if (!(hostCPU->name = virXMLPropString(hostCPUNode, "model"))) {
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("missing host CPU model name in QEMU "
+ "capabilities cache"));
+ goto cleanup;
}
ctxt->node = hostCPUNode;
@@ -3170,17 +3174,17 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
hostCPU->nprops = n;
for (i = 0; i < n; i++) {
- if (!(str = virXMLPropString(featureNodes[i], "name"))) {
+ hostCPU->props[i].name = virXMLPropString(featureNodes[i], "name");
+ if (!hostCPU->props[i].name) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("missing 'name' element for a host CPU model"
- " feature in QEMU capabilities cache"));
+ _("missing 'name' attribute for a host CPU"
+ " model feature in QEMU capabilities cache"));
goto cleanup;
}
- if (VIR_STRDUP(hostCPU->props[i].name, str) < 0)
- goto cleanup;
+
if (!(str = virXMLPropString(featureNodes[i], "supported"))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("missing 'supported' element for a host CPU"
+ _("missing 'supported' attribute for a host CPU"
" model feature in QEMU capabilities cache"));
goto cleanup;
}
@@ -3189,11 +3193,11 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
} else if (STREQ(str, "no")) {
hostCPU->props[i].supported = false;
} else {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("malformed supported value: expected 'yes'"
- " or 'no'"));
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("invalid supported value: '%s'"), str);
goto cleanup;
}
+ VIR_FREE(str);
}
}
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
