Re: [PATCH 7/9] qemu-caps: Get host model directly from Qemu when available

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux