Re: [libvirt] [PATCHv2 07/11] qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo conversions
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
Re: [libvirt] [PATCHv2 07/11] qemu_capabilities: Introduce virCPUDef / qemuMonitorCPUModelInfo conversions
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_o
