[libvirt] [PATCHv2 07/11] qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo conversions

Chris Venteicher posted 11 patches 7 years, 7 months ago
There is a newer version of this series
[libvirt] [PATCHv2 07/11] qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo conversions
Posted by Chris Venteicher 7 years, 7 months ago
Bi-directional conversion functions.
Converts model / name and features / properties between the two structures.

Created reusable functions to bridge the internal (virCpuDef) and
QEMU/QMP specific structures for describing CPU Models.
---
 src/qemu/qemu_capabilities.c | 146 +++++++++++++++++++++++++++++------
 src/qemu/qemu_capabilities.h |   3 +
 2 files changed, 127 insertions(+), 22 deletions(-)

diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 72ab012a78..d3f2317a1d 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -2758,7 +2758,8 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
                             virCPUDefPtr cpu,
                             bool migratable)
 {
-    size_t i;
+    virCPUDefPtr tmp = NULL;
+    int ret = -1;
 
     if (!modelInfo) {
         if (type == VIR_DOMAIN_VIRT_KVM) {
@@ -2771,32 +2772,21 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
         return 2;
     }
 
-    if (VIR_STRDUP(cpu->model, modelInfo->name) < 0 ||
-        VIR_ALLOC_N(cpu->features, modelInfo->nprops) < 0)
-        return -1;
+    if (!(tmp = virQEMUCapsCPUModelInfoToCPUDef(migratable, modelInfo)))
+        goto cleanup;
 
-    cpu->nfeatures_max = modelInfo->nprops;
-    cpu->nfeatures = 0;
+    /* Free then copy over model, vendor, vendor_id and features */
+    virCPUDefFreeModel(cpu);
 
-    for (i = 0; i < modelInfo->nprops; i++) {
-        virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
-        qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
-
-        if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
-            continue;
+    if (virCPUDefCopyModel(cpu, tmp, true) < 0)
+        goto cleanup;
 
-        if (VIR_STRDUP(feature->name, prop->name) < 0)
-            return -1;
+    ret = 0;
 
-        if (!prop->value.boolean ||
-            (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO))
-            feature->policy = VIR_CPU_FEATURE_DISABLE;
-        else
-            feature->policy = VIR_CPU_FEATURE_REQUIRE;
-        cpu->nfeatures++;
-    }
+ cleanup:
+    virCPUDefFree(tmp);
 
-    return 0;
+    return ret;
 }
 
 
@@ -3547,6 +3537,118 @@ virQEMUCapsLoadCache(virArch hostArch,
     return ret;
 }
 
+/* virCPUDef model    => qemuMonitorCPUModelInfo name
+ * virCPUDef features => qemuMonitorCPUModelInfo boolean properties */
+qemuMonitorCPUModelInfoPtr
+virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef)
+{
+    size_t i;
+    qemuMonitorCPUModelInfoPtr cpuModel = NULL;
+    qemuMonitorCPUModelInfoPtr ret = NULL;
+
+    if (!cpuDef || (VIR_ALLOC(cpuModel) < 0))
+        goto cleanup;
+
+    VIR_DEBUG("cpuDef->model = %s", NULLSTR(cpuDef->model));
+
+    if (VIR_STRDUP(cpuModel->name, cpuDef->model) < 0 ||
+        VIR_ALLOC_N(cpuModel->props, cpuDef->nfeatures) < 0)
+        goto cleanup;
+
+    /* no visibility on migratability of properties / features */
+    cpuModel->props_migratable_valid = false;
+
+    cpuModel->nprops = 0;
+
+    for (i = 0; i < cpuDef->nfeatures; i++) {
+        qemuMonitorCPUPropertyPtr prop = &(cpuModel->props[cpuModel->nprops]);
+        virCPUFeatureDefPtr feature = &(cpuDef->features[i]);
+
+        if (!feature || !(feature->name))
+            continue;
+
+        if (VIR_STRDUP(prop->name, feature->name) < 0)
+            goto cleanup;
+
+        prop->migratable = VIR_TRISTATE_BOOL_ABSENT;
+        prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
+
+        switch (feature->policy) {
+        case -1:                        /* policy undefined */
+        case VIR_CPU_FEATURE_FORCE:
+        case VIR_CPU_FEATURE_REQUIRE:
+            prop->value.boolean = true;
+            break;
+
+        case VIR_CPU_FEATURE_FORBID:
+        case VIR_CPU_FEATURE_DISABLE:
+        case VIR_CPU_FEATURE_OPTIONAL:
+        case VIR_CPU_FEATURE_LAST:
+            prop->value.boolean = false;
+            break;
+        }
+
+        cpuModel->nprops += 1;
+    }
+
+    VIR_STEAL_PTR(ret, cpuModel);
+
+ cleanup:
+    qemuMonitorCPUModelInfoFree(cpuModel);
+    return ret;
+}
+
+
+/* qemuMonitorCPUModelInfo name               => virCPUDef model
+ * qemuMonitorCPUModelInfo boolean properties => virCPUDef features
+ *
+ * migratable_only true: mark non-migratable features as disabled
+ *                 false: allow all features as required
+ */
+virCPUDefPtr
+virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model)
+{
+    virCPUDefPtr cpu = NULL;
+    virCPUDefPtr ret = NULL;
+    size_t i;
+
+    if (!model || (VIR_ALLOC(cpu) < 0))
+        goto cleanup;
+
+    VIR_DEBUG("model->name= %s", NULLSTR(model->name));
+
+    if (VIR_STRDUP(cpu->model, model->name) < 0 ||
+        VIR_ALLOC_N(cpu->features, model->nprops) < 0)
+        goto cleanup;
+
+    cpu->nfeatures_max = model->nprops;
+    cpu->nfeatures = 0;
+
+    for (i = 0; i < model->nprops; i++) {
+        virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
+        qemuMonitorCPUPropertyPtr prop = model->props + i;
+
+        if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
+            continue;
+
+        if (VIR_STRDUP(feature->name, prop->name) < 0)
+            goto cleanup;
+
+        if (!prop->value.boolean ||
+            (migratable_only && prop->migratable == VIR_TRISTATE_BOOL_NO))
+            feature->policy = VIR_CPU_FEATURE_DISABLE;
+        else
+            feature->policy = VIR_CPU_FEATURE_REQUIRE;
+
+        cpu->nfeatures++;
+    }
+
+    VIR_STEAL_PTR(ret, cpu);
+
+ cleanup:
+    virCPUDefFree(cpu);
+    return ret;
+}
 
 static void
 virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
index a048a1cf02..d5cd486295 100644
--- a/src/qemu/qemu_capabilities.h
+++ b/src/qemu/qemu_capabilities.h
@@ -564,6 +564,9 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
 void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps,
                                     const char *machineType);
 
+qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef);
+virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model);
+
 virFileCachePtr virQEMUCapsCacheNew(const char *libDir,
                                     const char *cacheDir,
                                     uid_t uid,
-- 
2.17.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 07/11] qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo conversions
Posted by Collin Walling 7 years, 7 months ago
On 07/09/2018 11:56 PM, Chris Venteicher wrote:
> Bi-directional conversion functions.
> Converts model / name and features / properties between the two structures.
> 
> Created reusable functions to bridge the internal (virCpuDef) and
> QEMU/QMP specific structures for describing CPU Models.
> ---
>  src/qemu/qemu_capabilities.c | 146 +++++++++++++++++++++++++++++------
>  src/qemu/qemu_capabilities.h |   3 +
>  2 files changed, 127 insertions(+), 22 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 72ab012a78..d3f2317a1d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2758,7 +2758,8 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
>                              virCPUDefPtr cpu,
>                              bool migratable)
>  {
> -    size_t i;
> +    virCPUDefPtr tmp = NULL;
> +    int ret = -1;
>  
>      if (!modelInfo) {
>          if (type == VIR_DOMAIN_VIRT_KVM) {
> @@ -2771,32 +2772,21 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
>          return 2;
>      }
>  
> -    if (VIR_STRDUP(cpu->model, modelInfo->name) < 0 ||
> -        VIR_ALLOC_N(cpu->features, modelInfo->nprops) < 0)
> -        return -1;
> +    if (!(tmp = virQEMUCapsCPUModelInfoToCPUDef(migratable, modelInfo)))
> +        goto cleanup;
>  
> -    cpu->nfeatures_max = modelInfo->nprops;
> -    cpu->nfeatures = 0;
> +    /* Free then copy over model, vendor, vendor_id and features */
> +    virCPUDefFreeModel(cpu);
>  
> -    for (i = 0; i < modelInfo->nprops; i++) {
> -        virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> -        qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
> -
> -        if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> -            continue;
> +    if (virCPUDefCopyModel(cpu, tmp, true) < 0)
> +        goto cleanup;
>  
> -        if (VIR_STRDUP(feature->name, prop->name) < 0)
> -            return -1;
> +    ret = 0;
>  
> -        if (!prop->value.boolean ||
> -            (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO))
> -            feature->policy = VIR_CPU_FEATURE_DISABLE;
> -        else
> -            feature->policy = VIR_CPU_FEATURE_REQUIRE;
> -        cpu->nfeatures++;
> -    }
> + cleanup:
> +    virCPUDefFree(tmp);
>  
> -    return 0;
> +    return ret;
>  }
>  
>  
> @@ -3547,6 +3537,118 @@ virQEMUCapsLoadCache(virArch hostArch,
>      return ret;
>  }
>  

It might make sense to rename these to|from "CPUDefS390" or something, since
s390x discards the migratability fields and only considers CPU policies 
enabled and disabled.

Or maybe someone with a stronger understanding of other libvirt CPU defs can 
help make these functions more agnostic.

> +/* virCPUDef model    => qemuMonitorCPUModelInfo name
> + * virCPUDef features => qemuMonitorCPUModelInfo boolean properties */
> +qemuMonitorCPUModelInfoPtr
> +virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef)
> +{
> +    size_t i;
> +    qemuMonitorCPUModelInfoPtr cpuModel = NULL;
> +    qemuMonitorCPUModelInfoPtr ret = NULL;
> +
> +    if (!cpuDef || (VIR_ALLOC(cpuModel) < 0))
> +        goto cleanup;
> +
> +    VIR_DEBUG("cpuDef->model = %s", NULLSTR(cpuDef->model));
> +
> +    if (VIR_STRDUP(cpuModel->name, cpuDef->model) < 0 ||
> +        VIR_ALLOC_N(cpuModel->props, cpuDef->nfeatures) < 0)
> +        goto cleanup;
> +
> +    /* no visibility on migratability of properties / features */
> +    cpuModel->props_migratable_valid = false;

I think this should be set to VIR_TRISTATE_BOOL_ABSENT (which evaluates to the same thing anyway)

> +
> +    cpuModel->nprops = 0;
> +
> +    for (i = 0; i < cpuDef->nfeatures; i++) {
> +        qemuMonitorCPUPropertyPtr prop = &(cpuModel->props[cpuModel->nprops]);
> +        virCPUFeatureDefPtr feature = &(cpuDef->features[i]);
> +
> +        if (!feature || !(feature->name))
> +            continue;
> +
> +        if (VIR_STRDUP(prop->name, feature->name) < 0)
> +            goto cleanup;
> +
> +        prop->migratable = VIR_TRISTATE_BOOL_ABSENT;

Set this to false.

> +        prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> +
> +        switch (feature->policy) {
> +        case -1:                        /* policy undefined */
> +        case VIR_CPU_FEATURE_FORCE:
> +        case VIR_CPU_FEATURE_REQUIRE:
> +            prop->value.boolean = true;
> +            break;
> +
> +        case VIR_CPU_FEATURE_FORBID:
> +        case VIR_CPU_FEATURE_DISABLE:
> +        case VIR_CPU_FEATURE_OPTIONAL:
> +        case VIR_CPU_FEATURE_LAST:
> +            prop->value.boolean = false;
> +            break;
> +        }
> +
> +        cpuModel->nprops += 1;
> +    }
> +
> +    VIR_STEAL_PTR(ret, cpuModel);
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(cpuModel);
> +    return ret;
> +}
> +
> +
> +/* qemuMonitorCPUModelInfo name               => virCPUDef model
> + * qemuMonitorCPUModelInfo boolean properties => virCPUDef features
> + *
> + * migratable_only true: mark non-migratable features as disabled
> + *                 false: allow all features as required
> + */
> +virCPUDefPtr
> +virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model)
> +{
> +    virCPUDefPtr cpu = NULL;
> +    virCPUDefPtr ret = NULL;
> +    size_t i;
> +
> +    if (!model || (VIR_ALLOC(cpu) < 0))
> +        goto cleanup;
> +
> +    VIR_DEBUG("model->name= %s", NULLSTR(model->name));
> +
> +    if (VIR_STRDUP(cpu->model, model->name) < 0 ||
> +        VIR_ALLOC_N(cpu->features, model->nprops) < 0)
> +        goto cleanup;
> +
> +    cpu->nfeatures_max = model->nprops;
> +    cpu->nfeatures = 0;
> +
> +    for (i = 0; i < model->nprops; i++) {
> +        virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> +        qemuMonitorCPUPropertyPtr prop = model->props + i;
> +
> +        if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> +            continue;
> +
> +        if (VIR_STRDUP(feature->name, prop->name) < 0)
> +            goto cleanup;
> +
> +        if (!prop->value.boolean ||
> +            (migratable_only && prop->migratable == VIR_TRISTATE_BOOL_NO))
> +            feature->policy = VIR_CPU_FEATURE_DISABLE;
> +        else
> +            feature->policy = VIR_CPU_FEATURE_REQUIRE;
> +
> +        cpu->nfeatures++;
> +    }
> +
> +    VIR_STEAL_PTR(ret, cpu);
> +
> + cleanup:
> +    virCPUDefFree(cpu);
> +    return ret;
> +}
>  
>  static void
>  virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index a048a1cf02..d5cd486295 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -564,6 +564,9 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
>  void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps,
>                                      const char *machineType);
>  
> +qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef);
> +virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model);
> +
>  virFileCachePtr virQEMUCapsCacheNew(const char *libDir,
>                                      const char *cacheDir,
>                                      uid_t uid,
> 


-- 
Respectfully,
- Collin Walling

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCHv2 07/11] qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo conversions
Posted by Jiri Denemark 7 years, 6 months ago
On Mon, Jul 09, 2018 at 22:56:51 -0500, Chris Venteicher wrote:
> Bi-directional conversion functions.
> Converts model / name and features / properties between the two structures.
> 
> Created reusable functions to bridge the internal (virCpuDef) and

s/virCpuDef/virCPUDef/

> QEMU/QMP specific structures for describing CPU Models.
> ---
>  src/qemu/qemu_capabilities.c | 146 +++++++++++++++++++++++++++++------
>  src/qemu/qemu_capabilities.h |   3 +
>  2 files changed, 127 insertions(+), 22 deletions(-)
> 
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 72ab012a78..d3f2317a1d 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2758,7 +2758,8 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
>                              virCPUDefPtr cpu,
>                              bool migratable)
>  {
> -    size_t i;
> +    virCPUDefPtr tmp = NULL;
> +    int ret = -1;
>  
>      if (!modelInfo) {
>          if (type == VIR_DOMAIN_VIRT_KVM) {
> @@ -2771,32 +2772,21 @@ virQEMUCapsInitCPUModelS390(virQEMUCapsPtr qemuCaps,
>          return 2;
>      }
>  
> -    if (VIR_STRDUP(cpu->model, modelInfo->name) < 0 ||
> -        VIR_ALLOC_N(cpu->features, modelInfo->nprops) < 0)
> -        return -1;
> +    if (!(tmp = virQEMUCapsCPUModelInfoToCPUDef(migratable, modelInfo)))
> +        goto cleanup;
>  
> -    cpu->nfeatures_max = modelInfo->nprops;
> -    cpu->nfeatures = 0;
> +    /* Free then copy over model, vendor, vendor_id and features */
> +    virCPUDefFreeModel(cpu);
>  
> -    for (i = 0; i < modelInfo->nprops; i++) {
> -        virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> -        qemuMonitorCPUPropertyPtr prop = modelInfo->props + i;
> -
> -        if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> -            continue;
> +    if (virCPUDefCopyModel(cpu, tmp, true) < 0)
> +        goto cleanup;

You can replace the virCPUDefFreeModel/virCPUDefCopyModel combo with a
single virCPUDefStealModel.

>  
> -        if (VIR_STRDUP(feature->name, prop->name) < 0)
> -            return -1;
> +    ret = 0;
>  
> -        if (!prop->value.boolean ||
> -            (migratable && prop->migratable == VIR_TRISTATE_BOOL_NO))
> -            feature->policy = VIR_CPU_FEATURE_DISABLE;
> -        else
> -            feature->policy = VIR_CPU_FEATURE_REQUIRE;
> -        cpu->nfeatures++;
> -    }
> + cleanup:
> +    virCPUDefFree(tmp);
>  
> -    return 0;
> +    return ret;
>  }
>  
>  
> @@ -3547,6 +3537,118 @@ virQEMUCapsLoadCache(virArch hostArch,
>      return ret;
>  }
>  
> +/* virCPUDef model    => qemuMonitorCPUModelInfo name
> + * virCPUDef features => qemuMonitorCPUModelInfo boolean properties */
> +qemuMonitorCPUModelInfoPtr
> +virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef)
> +{
> +    size_t i;
> +    qemuMonitorCPUModelInfoPtr cpuModel = NULL;
> +    qemuMonitorCPUModelInfoPtr ret = NULL;
> +
> +    if (!cpuDef || (VIR_ALLOC(cpuModel) < 0))
> +        goto cleanup;
> +
> +    VIR_DEBUG("cpuDef->model = %s", NULLSTR(cpuDef->model));
> +
> +    if (VIR_STRDUP(cpuModel->name, cpuDef->model) < 0 ||
> +        VIR_ALLOC_N(cpuModel->props, cpuDef->nfeatures) < 0)
> +        goto cleanup;
> +
> +    /* no visibility on migratability of properties / features */
> +    cpuModel->props_migratable_valid = false;

I don't think there's any need to set this explicitly as this is the
default value.

> +
> +    cpuModel->nprops = 0;

This is also the default value, but since we actually care about the
value and increment it later, it's useful to explicitly set it anyway.
(In contrast to the migratability bool above)

> +
> +    for (i = 0; i < cpuDef->nfeatures; i++) {
> +        qemuMonitorCPUPropertyPtr prop = &(cpuModel->props[cpuModel->nprops]);
> +        virCPUFeatureDefPtr feature = &(cpuDef->features[i]);
> +
> +        if (!feature || !(feature->name))
> +            continue;

feature cannot be NULL at this point.

> +
> +        if (VIR_STRDUP(prop->name, feature->name) < 0)
> +            goto cleanup;
> +
> +        prop->migratable = VIR_TRISTATE_BOOL_ABSENT;

No need to set this explicitly.

> +        prop->type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> +
> +        switch (feature->policy) {
> +        case -1:                        /* policy undefined */
> +        case VIR_CPU_FEATURE_FORCE:
> +        case VIR_CPU_FEATURE_REQUIRE:
> +            prop->value.boolean = true;
> +            break;
> +
> +        case VIR_CPU_FEATURE_FORBID:
> +        case VIR_CPU_FEATURE_DISABLE:
> +        case VIR_CPU_FEATURE_OPTIONAL:
> +        case VIR_CPU_FEATURE_LAST:
> +            prop->value.boolean = false;
> +            break;
> +        }

I don't see any value in using switch since the compiler won't warn us
in case of missing case anyway because feature->policy is int. I'd just
write it as

    if (feature->policy == -1 ||
        feature->policy == VIR_CPU_FEATURE_FORCE ||
        feature->policy == VIR_CPU_FEATURE_REQUIRE)
        prop->value.boolean = true;

or perhaps even better using direct assignment

    prop->value.boolean = feature->policy == -1 ||
                          feature->policy == VIR_CPU_FEATURE_FORCE ||
                          feature->policy == VIR_CPU_FEATURE_REQUIRE;
> +
> +        cpuModel->nprops += 1;

cpuModel->nprops++;

> +    }
> +
> +    VIR_STEAL_PTR(ret, cpuModel);
> +
> + cleanup:
> +    qemuMonitorCPUModelInfoFree(cpuModel);
> +    return ret;
> +}
> +
> +
> +/* qemuMonitorCPUModelInfo name               => virCPUDef model
> + * qemuMonitorCPUModelInfo boolean properties => virCPUDef features
> + *
> + * migratable_only true: mark non-migratable features as disabled
> + *                 false: allow all features as required
> + */
> +virCPUDefPtr
> +virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model)

One parameter per line. And I think "migratable" would be as descriptive
as "migratable_only" but shorter.

> +{
> +    virCPUDefPtr cpu = NULL;
> +    virCPUDefPtr ret = NULL;
> +    size_t i;
> +
> +    if (!model || (VIR_ALLOC(cpu) < 0))
> +        goto cleanup;

This would return an error without reporting any error if model == NULL.
But do we really need to check for that?

And redundant parentheses again.

> +
> +    VIR_DEBUG("model->name= %s", NULLSTR(model->name));
> +
> +    if (VIR_STRDUP(cpu->model, model->name) < 0 ||
> +        VIR_ALLOC_N(cpu->features, model->nprops) < 0)
> +        goto cleanup;
> +
> +    cpu->nfeatures_max = model->nprops;
> +    cpu->nfeatures = 0;
> +
> +    for (i = 0; i < model->nprops; i++) {
> +        virCPUFeatureDefPtr feature = cpu->features + cpu->nfeatures;
> +        qemuMonitorCPUPropertyPtr prop = model->props + i;
> +
> +        if (prop->type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN)
> +            continue;
> +
> +        if (VIR_STRDUP(feature->name, prop->name) < 0)
> +            goto cleanup;
> +
> +        if (!prop->value.boolean ||
> +            (migratable_only && prop->migratable == VIR_TRISTATE_BOOL_NO))

Again, I don't see a reason for renaming migratable to migratable_only.

> +            feature->policy = VIR_CPU_FEATURE_DISABLE;
> +        else
> +            feature->policy = VIR_CPU_FEATURE_REQUIRE;
> +
> +        cpu->nfeatures++;
> +    }
> +
> +    VIR_STEAL_PTR(ret, cpu);
> +
> + cleanup:
> +    virCPUDefFree(cpu);
> +    return ret;
> +}
>  
>  static void
>  virQEMUCapsFormatHostCPUModelInfo(virQEMUCapsPtr qemuCaps,
> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> index a048a1cf02..d5cd486295 100644
> --- a/src/qemu/qemu_capabilities.h
> +++ b/src/qemu/qemu_capabilities.h
> @@ -564,6 +564,9 @@ int virQEMUCapsGetMachineTypesCaps(virQEMUCapsPtr qemuCaps,
>  void virQEMUCapsFilterByMachineType(virQEMUCapsPtr qemuCaps,
>                                      const char *machineType);
>  
> +qemuMonitorCPUModelInfoPtr virQEMUCapsCPUModelInfoFromCPUDef(const virCPUDef *cpuDef);
> +virCPUDefPtr virQEMUCapsCPUModelInfoToCPUDef(bool migratable_only, qemuMonitorCPUModelInfoPtr model);

One parameter per line. Anyway somehow I feel these new functions do not
belong to qemu_capabilities.h. But I'm not sure what would be the better
place for them yet.

Jirka

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