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@linux.vnet.ibm.com> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com>
# Conflicts: # tests/qemucapabilitiesdata/caps_2.8.0.s390x.replies
Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> --- 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); } }