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