On Mon, Jul 09, 2018 at 22:56:49 -0500, Chris Venteicher wrote:
> Send both model name and a set of features/properties to QEMU for
> expansion rather than just the model name.
>
> Required to expand name+props models of the form computed by baseline
> into fully expanded (all props/features listed) output.
This patch is doing too much at once and is quite hard to review.
> ---
> src/qemu/qemu_capabilities.c | 42 +++++++++++++++++-----
> src/qemu/qemu_monitor.c | 38 ++++++++++++++++----
> src/qemu/qemu_monitor.h | 5 ++-
> src/qemu/qemu_monitor_json.c | 69 ++++++++++++++++++++++++------------
> src/qemu/qemu_monitor_json.h | 7 ++--
> tests/cputest.c | 7 +++-
> 6 files changed, 122 insertions(+), 46 deletions(-)
>
> diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> index 3d78e2e29b..72ab012a78 100644
> --- a/src/qemu/qemu_capabilities.c
> +++ b/src/qemu/qemu_capabilities.c
> @@ -2343,23 +2343,32 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
> qemuMonitorCPUModelInfoPtr modelInfo = NULL;
> qemuMonitorCPUModelInfoPtr nonMigratable = NULL;
> virHashTablePtr hash = NULL;
> - const char *model;
> + const char *model_name;
Why? If we really want to do this, it should go into a separate patch
explaining why it is needed.
> qemuMonitorCPUModelExpansionType type;
> virDomainVirtType virtType;
> virQEMUCapsHostCPUDataPtr cpuData;
> int ret = -1;
> + int err = -1;
We usually call such variable 'rc' and leave it uninitialized.
>
> if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_EXPANSION))
> return 0;
>
> if (tcg || !virQEMUCapsGet(qemuCaps, QEMU_CAPS_KVM)) {
> virtType = VIR_DOMAIN_VIRT_QEMU;
> - model = "max";
> + model_name = "max";
> } else {
> virtType = VIR_DOMAIN_VIRT_KVM;
> - model = "host";
> + model_name = "host";
> }
>
> + if ((VIR_ALLOC(modelInfo) < 0) ||
> + (VIR_ALLOC(nonMigratable) < 0))
> + goto cleanup;
> +
> + if ((qemuMonitorCPUModelInfoInit(model_name, modelInfo) < 0) ||
> + (qemuMonitorCPUModelInfoInit(model_name, nonMigratable) < 0))
> + goto cleanup;
Redundant parentheses. But anyway, VIR_ALLOC +
qemuMonitorCPUModelInfoInit combo should be replaced with a single
qemuMonitorCPUModelInfoNew function which would do both. As a bonus
point, you could never end up with a non-NULL modelInfo structure with
name == NULL.
> +
> cpuData = virQEMUCapsGetHostCPUData(qemuCaps, virtType);
>
> /* Some x86_64 features defined in cpu_map.xml use spelling which differ
> @@ -2372,16 +2381,31 @@ virQEMUCapsProbeQMPHostCPU(virQEMUCapsPtr qemuCaps,
> else
> type = QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC;
>
> - if (qemuMonitorGetCPUModelExpansion(mon, type, model, true, &modelInfo) < 0)
> + if ((err = qemuMonitorGetCPUModelExpansion(mon, type, true, modelInfo)) < 0)
> goto cleanup;
>
> - /* Try to check migratability of each feature. */
> - if (modelInfo &&
> - qemuMonitorGetCPUModelExpansion(mon, type, model, false,
> - &nonMigratable) < 0)
> + if (err == 1) {
> + ret = 0; /* Qemu can't do expansion 1, exit without error */
> + goto cleanup; /* We don't have info so don't update cpuData->info */
> + }
It's not very clear to me why qemuMonitorGetCPUModelExpansion needs to
report 1. A separate patch with an explanation would be better.
> +
> + if ((err = qemuMonitorGetCPUModelExpansion(mon, type, false, nonMigratable)) < 0)
> goto cleanup;
>
> - if (nonMigratable) {
> + /* Try to check migratability of each feature */
> + /* Expansion 1 sets migratable features true
> + * Expansion 2 sets migratable and non-migratable features true
> + * (non-migratable set true only in some archs like X86)
> + *
> + * If delta between Expansion 1 and 2 exists...
> + * - both migratable and non-migratable features set prop->value = true
> + * - migratable features set prop->migatable = VIR_TRISTATE_BOOL_YES
> + * - non-migratable features set prop->migatable = VIR_TRISTATE_BOOL_NO
> + */
> + if (err == 0) {
> + /* Expansion 2 succeded
> + * Qemu expanded both migratable and nonMigratable features */
This comment seems redundant. It's pretty clear from the code that both
expansions were successful at this point.
> +
> qemuMonitorCPUPropertyPtr prop;
> qemuMonitorCPUPropertyPtr nmProp;
> size_t i;
> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
> index 2d9297c3a7..91b946c8b4 100644
> --- a/src/qemu/qemu_monitor.c
> +++ b/src/qemu/qemu_monitor.c
> @@ -3619,20 +3619,46 @@ qemuMonitorCPUDefInfoFree(qemuMonitorCPUDefInfoPtr cpu)
> }
>
>
> +/*
> + * type static:
> + * Expand to static base model + delta property changes
> + * Returned model is invariant and migration safe
> + *
> + * model_info->name = base model name
> + * model_info->props = features to +/- to base model to achive model_name
> + *
> + * type full:
> + * Expand model to enumerate all properties
> + * Returned model isn't guaranteed to be invariant or migration safe.
> + *
> + * model_info->name = base model name
> + * model_info->props = features to +/- to empty set to achive model_name
> + *
> + * type static_full:
> + * Expand to static base model + delta property changes (pass 0)
> + * Expand model to enumerate all properties (pass 1)
> + * Returned model is invariant and migration safe
> + *
> + * model_info->name = base model name
> + * model_info->props = features to +/- to empty set to achive model_name
> + *
> + * migratable_only:
> + * true: QEMU excludes non-migratable features
> + * false: QEMU includes non-migratable features for some archs like X86
> + */
Function description in our public API format and using real
qemuMonitorCPUModelExpansionType values would be a lot better.
> int
> qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
> qemuMonitorCPUModelExpansionType type,
> - const char *model_name,
> - bool migratable,
> - qemuMonitorCPUModelInfoPtr *model_info)
> + bool migratable_only,
Why do you need to rename migratable as migratable_only? The migratable
bool selects whether the CPU model returned by QEMU has to be migratable
or not. In any case it would be a separate change.
> + qemuMonitorCPUModelInfoPtr model_info)
If you keep the double pointer there, you could remove some hacks and
comments to explain these hacks in the JSON monitor implementation. And
you wouldn't need to introduce qemuMonitorCPUModelInfoFreeContents.
> {
> VIR_DEBUG("type=%d model_name=%s migratable=%d",
> - type, model_name, migratable);
> + type, (model_info ? NULLSTR(model_info->name):"NULL"),
The function is not supposed to be called with model_info == NULL, the
check here is useless. Not to mention that if model_info was NULL,
qemuMonitorJSONGetCPUModelExpansion would segfault anyway.
And I believe the model_info->name can't be NULL either.
All this would of course change with the double pointer and additionally
this function should check a proper value was passed in.
> + migratable_only);
>
> QEMU_CHECK_MONITOR(mon);
>
> - return qemuMonitorJSONGetCPUModelExpansion(mon, type, model_name,
> - migratable, model_info);
> + return qemuMonitorJSONGetCPUModelExpansion(mon, type, migratable_only, model_info);
> }
>
>
> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
> index 0b84a91fbc..6b4b527512 100644
> --- a/src/qemu/qemu_monitor.h
> +++ b/src/qemu/qemu_monitor.h
> @@ -1016,9 +1016,8 @@ typedef enum {
>
> int qemuMonitorGetCPUModelExpansion(qemuMonitorPtr mon,
> qemuMonitorCPUModelExpansionType type,
> - const char *model_name,
> - bool migratable,
> - qemuMonitorCPUModelInfoPtr *model_info);
> + bool migratable_only,
> + qemuMonitorCPUModelInfoPtr model_info);
>
> void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info);
> void qemuMonitorCPUModelInfoFreeContents(qemuMonitorCPUModelInfoPtr model_info);
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 90d43eee97..9b681f4592 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5390,8 +5390,11 @@ qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model)
> }
> }
>
> - ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name,
> - "a:props", &cpu_props, NULL));
> + if (model->nprops > 0)
> + ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name,
> + "a:props", &cpu_props, NULL));
> + else
> + ignore_value(virJSONValueObjectCreate(&model_json, "s:name", model->name, NULL));
Just don't fill in cpu_props at all and then you can use "A:props" to
cover both cases. Anyway, this can be done in a separate patch.
>
> cleanup:
> virJSONValueFree(cpu_props);
> @@ -5440,38 +5443,52 @@ qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr cpu_model)
> return model;
> }
>
> +
> +/* return:
> + * -1 - Execution Failure
> + * 0 - Success
> + * 1 - Qemu unable to do expansion leaving "model" unmodified
> + */
> int
> qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> qemuMonitorCPUModelExpansionType type,
> - const char *model_name,
> - bool migratable,
> - qemuMonitorCPUModelInfoPtr *model_info)
> + bool migratable_only,
> + qemuMonitorCPUModelInfoPtr model)
> {
> int ret = -1;
> - virJSONValuePtr model = NULL;
> - virJSONValuePtr props = NULL;
> + virJSONValuePtr json_model = NULL;
> virJSONValuePtr cmd = NULL;
> virJSONValuePtr reply = NULL;
> virJSONValuePtr data;
> virJSONValuePtr cpu_model;
> + qemuMonitorCPUModelInfoPtr expanded_model = NULL;
> + qemuMonitorCPUModelInfoPtr model_info = NULL;
> const char *typeStr = "";
>
> - *model_info = NULL;
> + if (!(model_info = qemuMonitorCPUModelInfoCopy(model)))
> + return -1;
> +
> + qemuMonitorCPUModelInfoFreeContents(model);
We usually don't touch output parameters until we know the function
succeeded. Double pointer would let you comply with this.
>
> - if (!(model = virJSONValueNewObject()))
> - goto cleanup;
> + if (!migratable_only) {
> + /* Add property to input CPUModelInfo causing QEMU to include
> + * non-migratable properties for some architectures like X86 */
This comment doesn't really belong here. It's the caller's decision to
run this command with appropriate arguments for each architecture.
>
> - if (virJSONValueObjectAppendString(model, "name", model_name) < 0)
> - goto cleanup;
> + qemuMonitorCPUProperty prop;
> + prop.type = QEMU_MONITOR_CPU_PROPERTY_BOOLEAN;
> + prop.value.boolean = false;
> + prop.migratable = false;
> +
> + if (VIR_STRDUP(prop.name, "migratable") < 0)
> + goto cleanup;
>
> - if (!migratable) {
> - if (!(props = virJSONValueNewObject()) ||
> - virJSONValueObjectAppendBoolean(props, "migratable", false) < 0 ||
> - virJSONValueObjectAppend(model, "props", props) < 0)
> + if (VIR_APPEND_ELEMENT(model_info->props, model_info->nprops, prop) < 0)
You'd leak prop.name here.
> goto cleanup;
> - props = NULL;
> }
>
> + if (!(json_model = qemuMonitorJSONBuildCPUModelInfoToJSON(model_info)))
> + goto cleanup;
> +
> retry:
> switch (type) {
> case QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC:
> @@ -5486,7 +5503,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
>
> if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-expansion",
> "s:type", typeStr,
> - "a:model", &model,
> + "a:model", &json_model,
> NULL)))
> goto cleanup;
>
> @@ -5498,7 +5515,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> * guest architecture or it is not supported in the host environment.
> */
> if (qemuMonitorJSONHasError(reply, "GenericError")) {
> - ret = 0;
> + ret = 1;
> goto cleanup;
> }
>
> @@ -5517,7 +5534,9 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> * on the result of the initial "static" expansion.
> */
> if (type == QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC_FULL) {
> - if (!(model = virJSONValueCopy(cpu_model)))
> + virJSONValueFree(json_model);
> +
> + if (!(json_model = virJSONValueCopy(cpu_model)))
> goto cleanup;
>
> virJSONValueFree(cmd);
> @@ -5526,16 +5545,20 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> goto retry;
> }
>
> - if (!(*model_info = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model)))
> + if (!(expanded_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(cpu_model)))
> goto cleanup;
>
> + *model = *expanded_model; /* overwrite contents */
> +
> ret = 0;
>
> cleanup:
> + VIR_FREE(expanded_model); /* Free structure but not reused contents */
> + qemuMonitorCPUModelInfoFreeContents(model_info);
> +
> virJSONValueFree(cmd);
> virJSONValueFree(reply);
> - virJSONValueFree(model);
> - virJSONValueFree(props);
> + virJSONValueFree(json_model);
> return ret;
> }
>
> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
> index 73e1cb6ace..9950483c5c 100644
> --- a/src/qemu/qemu_monitor_json.h
> +++ b/src/qemu/qemu_monitor_json.h
> @@ -362,10 +362,9 @@ int qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>
> int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
> qemuMonitorCPUModelExpansionType type,
> - const char *model_name,
> - bool migratable,
> - qemuMonitorCPUModelInfoPtr *model_info)
> - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5);
> + bool migratable_only,
> + qemuMonitorCPUModelInfoPtr model_info)
> + ATTRIBUTE_NONNULL(4);
>
> int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon,
> qemuMonitorCPUModelInfoPtr model_a,
> diff --git a/tests/cputest.c b/tests/cputest.c
> index baf2b3c648..27727aa29e 100644
> --- a/tests/cputest.c
> +++ b/tests/cputest.c
> @@ -495,9 +495,14 @@ cpuTestMakeQEMUCaps(const struct data *data)
> if (!(testMon = qemuMonitorTestNewFromFile(json, driver.xmlopt, true)))
> goto error;
>
> + if ((VIR_ALLOC(model) < 0) ||
> + (qemuMonitorCPUModelInfoInit("host", model) < 0))
> + goto cleanup;
> +
> +
One empty line would be enough.
> if (qemuMonitorGetCPUModelExpansion(qemuMonitorTestGetMonitor(testMon),
> QEMU_MONITOR_CPU_MODEL_EXPANSION_STATIC,
> - "host", true, &model) < 0)
> + true, model) < 0)
> goto error;
>
> if (!(qemuCaps = virQEMUCapsNew()))
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
