Re: [libvirt] [PATCH v2 11/33] qemu: Store more types in qemuMonitorCPUModelInfo

[Jiri Denemark]

On Tue, Feb 21, 2017 at 09:27:45 -0500, John Ferlan wrote:
> 
> 
> On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> > While query-cpu-model-expansion returns only boolean features on s390,
> > but x86_64 reports some integer and string properties which we are
> > interested in.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> > 
> > Notes:
> > Version 2:
> > - no change
> > 
> >  src/qemu/qemu_capabilities.c | 84 
> > 
> >  src/qemu/qemu_monitor.c  | 22 ++-
> >  src/qemu/qemu_monitor.h  | 23 +--
> >  src/qemu/qemu_monitor_json.c | 37 ---
> >  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml |  7 ++
> >  5 files changed, 133 insertions(+), 40 deletions(-)
> > 
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index aab336954..466852d13 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -3074,14 +3074,16 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
> >  cpu->nfeatures = 0;
> >  
> >  for (i = 0; i < modelInfo->nprops; i++) {
> > -if (VIR_STRDUP(cpu->features[i].name, modelInfo->props[i].name) < 
> > 0)
> > +virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> > +qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
> > +
> > +if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> > +continue;
> 
> So s390 only supports "boolean" or "default" types?

S390 only supports boolean properties; there's no "default" type.

...
> > @@ -3187,30 +3188,47 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr 
> > qemuCaps,
> >  hostCPU->nprops = n;
> >  
> >  for (i = 0; i < n; i++) {
> > -hostCPU->props[i].name = virXMLPropString(featureNodes[i], 
> > "name");
> > -if (!hostCPU->props[i].name) {
> > +qemuMonitorCPUPropertyPtr prop = hostCPU->props + i;
> > +ctxt->node = featureNodes[i];
> > +
> > +if (!(prop->name = virXMLPropString(ctxt->node, "name"))) {
> >  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > _("missing 'name' attribute for a host CPU"
> >   " model property in QEMU capabilities 
> > cache"));
> >  goto cleanup;
> >  }
> 
> If you follow the suggestion I have in the previous patch, then if you
> don't find the property named "type", then you know the 'value' is
> either "yes" or "no"
> 
> If there is a type then the "value" property is either "string" or "number"

As explained in my previous reply, there's no need to support missing
type attribute.

...
> > diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> > index 8811d8501..112f041f1 100644
> > --- a/src/qemu/qemu_monitor.h
> > +++ b/src/qemu/qemu_monitor.h
> > @@ -921,16 +921,31 @@ int qemuMonitorGetCPUDefinitions(qemuMonitorPtr mon,
> >   qemuMonitorCPUDefInfoPtr **cpus);
> >  void qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu);
> >  
> > +typedef enum {
> > +QEMU_MONITOR_CPU_PROPERTY_BOOLEAN,
> 
> As stated in previous patch, I think should should be "DEFAULT"

No.

> 
> > +QEMU_MONITOR_CPU_PROPERTY_STRING,
> > +QEMU_MONITOR_CPU_PROPERTY_ULL,
> 
> likewise, "NUMBER" not "ULL"

Yes.

Jirka

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH v2 11/33] qemu: Store more types in qemuMonitorCPUModelInfo

[John Ferlan]

On 02/15/2017 11:44 AM, Jiri Denemark wrote:
> While query-cpu-model-expansion returns only boolean features on s390,
> but x86_64 reports some integer and string properties which we are
> interested in.
> 
> Signed-off-by: Jiri Denemark 
> ---
> 
> Notes:
> Version 2:
> - no change
> 
>  src/qemu/qemu_capabilities.c | 84 
> 
>  src/qemu/qemu_monitor.c  | 22 ++-
>  src/qemu/qemu_monitor.h  | 23 +--
>  src/qemu/qemu_monitor_json.c | 37 ---
>  tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml |  7 ++
>  5 files changed, 133 insertions(+), 40 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index aab336954..466852d13 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -3074,14 +3074,16 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
>  cpu->nfeatures = 0;
>  
>  for (i = 0; i < modelInfo->nprops; i++) {
> -if (VIR_STRDUP(cpu->features[i].name, modelInfo->props[i].name) < 0)
> +virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> +qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
> +
> +if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> +continue;

So s390 only supports "boolean" or "default" types?

> +
> +if (VIR_STRDUP(feature->name, prop->name) < 0)
>  return -1;
> -
> -if (modelInfo->props[i].supported)
> -cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
> -else
> -cpu->features[i].policy = VIR_CPU_FEATURE_DISABLE;
> -
> +feature->policy = prop->value.boolean ? VIR_CPU_FEATURE_REQUIRE
> +  : VIR_CPU_FEATURE_DISABLE;
>  cpu->nfeatures++;
>  }
>  
> @@ -3154,7 +3156,6 @@ static int
>  virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
>  xmlXPathContextPtr ctxt)
>  {
> -char *str = NULL;
>  xmlNodePtr hostCPUNode;
>  xmlNodePtr *featureNodes = NULL;
>  xmlNodePtr oldnode = ctxt->node;
> @@ -3187,30 +3188,47 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr 
> qemuCaps,
>  hostCPU->nprops = n;
>  
>  for (i = 0; i < n; i++) {
> -hostCPU->props[i].name = virXMLPropString(featureNodes[i], 
> "name");
> -if (!hostCPU->props[i].name) {
> +qemuMonitorCPUPropertyPtr prop = hostCPU->props + i;
> +ctxt->node = featureNodes[i];
> +
> +if (!(prop->name = virXMLPropString(ctxt->node, "name"))) {
>  virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("missing 'name' attribute for a host CPU"
>   " model property in QEMU capabilities 
> cache"));
>  goto cleanup;
>  }

If you follow the suggestion I have in the previous patch, then if you
don't find the property named "type", then you know the 'value' is
either "yes" or "no"

If there is a type then the "value" property is either "string" or "number"

>  
> -if (!(str = virXMLPropString(featureNodes[i], "boolean"))) {
> -virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> -   _("missing 'boolean' attribute for a host CPU"
> - " model property 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;
> +if (virXPathBoolean("boolean(./@boolean)", ctxt)) {
> +if (virXPathBoolean("./@boolean='yes'", ctxt))
> +prop->value.boolean = true;
> +prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> +} else if (virXPathBoolean("boolean(./@string)", ctxt)) {
> +prop->value.string = virXMLPropString(ctxt->node, "string");
> +if (!prop->value.string) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   _("invalid string value for '%s' host CPU 
> "
> + "model property in QEMU capabilities 
> cache"),
> +   prop->name);
> +goto cleanup;
> +}
> +prop->type = QEMU_MONITOR_CPU_PROPERTY_STRING;
> +} else if (virXPathBoolean("boolean(./@ull)", ctxt)) {

ull is just an implementation detail.  I think it should be number

> +if (virXPathULongLong("string(./@ull)", ctxt,
> +  >value.ull) < 0) {
> +virReportError(VIR_ERR_INTERNAL_ERROR,
> +   

[libvirt] [PATCH v2 11/33] qemu: Store more types in qemuMonitorCPUModelInfo

[Jiri Denemark]

While query-cpu-model-expansion returns only boolean features on s390,
but x86_64 reports some integer and string properties which we are
interested in.

Signed-off-by: Jiri Denemark 
---

Notes:
Version 2:
- no change

 src/qemu/qemu_capabilities.c | 84 
 src/qemu/qemu_monitor.c  | 22 ++-
 src/qemu/qemu_monitor.h  | 23 +--
 src/qemu/qemu_monitor_json.c | 37 ---
 tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml |  7 ++
 5 files changed, 133 insertions(+), 40 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index aab336954..466852d13 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -3074,14 +3074,16 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
 cpu->nfeatures = 0;
 
 for (i = 0; i < modelInfo->nprops; i++) {
-if (VIR_STRDUP(cpu->features[i].name, modelInfo->props[i].name) < 0)
+virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
+qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
+
+if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
+continue;
+
+if (VIR_STRDUP(feature->name, prop->name) < 0)
 return -1;
-
-if (modelInfo->props[i].supported)
-cpu->features[i].policy = VIR_CPU_FEATURE_REQUIRE;
-else
-cpu->features[i].policy = VIR_CPU_FEATURE_DISABLE;
-
+feature->policy = prop->value.boolean ? VIR_CPU_FEATURE_REQUIRE
+  : VIR_CPU_FEATURE_DISABLE;
 cpu->nfeatures++;
 }
 
@@ -3154,7 +3156,6 @@ static int
 virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
 xmlXPathContextPtr ctxt)
 {
-char *str = NULL;
 xmlNodePtr hostCPUNode;
 xmlNodePtr *featureNodes = NULL;
 xmlNodePtr oldnode = ctxt->node;
@@ -3187,30 +3188,47 @@ virQEMUCapsLoadHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
 hostCPU->nprops = n;
 
 for (i = 0; i < n; i++) {
-hostCPU->props[i].name = virXMLPropString(featureNodes[i], "name");
-if (!hostCPU->props[i].name) {
+qemuMonitorCPUPropertyPtr prop = hostCPU->props + i;
+ctxt->node = featureNodes[i];
+
+if (!(prop->name = virXMLPropString(ctxt->node, "name"))) {
 virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing 'name' attribute for a host CPU"
  " model property in QEMU capabilities 
cache"));
 goto cleanup;
 }
 
-if (!(str = virXMLPropString(featureNodes[i], "boolean"))) {
-virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
-   _("missing 'boolean' attribute for a host CPU"
- " model property 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;
+if (virXPathBoolean("boolean(./@boolean)", ctxt)) {
+if (virXPathBoolean("./@boolean='yes'", ctxt))
+prop->value.boolean = true;
+prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
+} else if (virXPathBoolean("boolean(./@string)", ctxt)) {
+prop->value.string = virXMLPropString(ctxt->node, "string");
+if (!prop->value.string) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("invalid string value for '%s' host CPU "
+ "model property in QEMU capabilities 
cache"),
+   prop->name);
+goto cleanup;
+}
+prop->type = QEMU_MONITOR_CPU_PROPERTY_STRING;
+} else if (virXPathBoolean("boolean(./@ull)", ctxt)) {
+if (virXPathULongLong("string(./@ull)", ctxt,
+  >value.ull) < 0) {
+virReportError(VIR_ERR_INTERNAL_ERROR,
+   _("invalid integer value for '%s' host CPU "
+ "model property in QEMU capabilities 
cache"),
+   prop->name);
+goto cleanup;
+}
+prop->type = QEMU_MONITOR_CPU_PROPERTY_ULL;
 } else {
 virReportError(VIR_ERR_INTERNAL_ERROR,
-   _("invalid boolean value: '%s'"), str);
+   _("missing value for '%s' host CPU model "
+ "property in QEMU capabilities cache"),
+