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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
