New function qemuMonitorJSONBuildCPUModelInfoFromJSON created by extracting code from existing function qemuMonitorJSONGetCPUModelExpansion to create a reusable function for extracting cpu model info from json. Function qemuMonitorJSONGetCPUModelInfoFromJSON returns qemuMonitorCPUModelInfoPtr on success and NULL on failure.

--- src/qemu/qemu_monitor_json.c | 78 +++++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 26 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 57c2c4de0..4368aaaa0 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5337,6 +5337,57 @@ qemuMonitorJSONParseCPUModelProperty(const char *key, return 0; }

+/* model_json: {"model": {"name": "IvyBridge", "props": {}}} + */ +static qemuMonitorCPUModelInfoPtr +qemuMonitorJSONBuildCPUModelInfoFromJSON(virJSONValuePtr model_json)

+{ + virJSONValuePtr cpu_model; + virJSONValuePtr cpu_props; + qemuMonitorCPUModelInfoPtr machine_model = NULL; + qemuMonitorCPUModelInfoPtr model = NULL; + char const *cpu_name; + + if (!(cpu_model = virJSONValueObjectGetObject(model_json, "model"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply data was missing 'model'"));

+ goto cleanup; + } + + if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply data was missing 'name'"));

+ goto cleanup; + } + + if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("query-cpu-model-expansion reply data was missing 'props'"));

+ goto cleanup; + } + + if (VIR_ALLOC(machine_model) < 0) + goto cleanup; + + if (VIR_STRDUP(machine_model->name, cpu_name) < 0) + goto cleanup; + + if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0) + goto cleanup; + + if (virJSONValueObjectForeachKeyValue(cpu_props, + qemuMonitorJSONParseCPUModelProperty, + machine_model) < 0) + goto cleanup; + + VIR_STEAL_PTR(model, machine_model); + + cleanup: + qemuMonitorCPUModelInfoFree(machine_model); + + return model; +} + int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelExpansionType type, @@ -5351,9 +5402,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, virJSONValuePtr reply = NULL; virJSONValuePtr data; virJSONValuePtr cpu_model; - virJSONValuePtr cpu_props; qemuMonitorCPUModelInfoPtr machine_model = NULL; - char const *cpu_name; const char *typeStr = ""; *model_info = NULL; @@ -5426,30 +5475,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, goto retry; } - if (!(cpu_name = virJSONValueObjectGetString(cpu_model, "name"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'name'")); - goto cleanup; - } - - if (!(cpu_props = virJSONValueObjectGetObject(cpu_model, "props"))) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("query-cpu-model-expansion reply data was missing 'props'")); - goto cleanup; - } - - if (VIR_ALLOC(machine_model) < 0) - goto cleanup; - - if (VIR_STRDUP(machine_model->name, cpu_name) < 0) - goto cleanup; - - if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props)) < 0) - goto cleanup; - - if (virJSONValueObjectForeachKeyValue(cpu_props, - qemuMonitorJSONParseCPUModelProperty, - machine_model) < 0) + if (!(machine_model = qemuMonitorJSONBuildCPUModelInfoFromJSON(data))) goto cleanup; ret = 0;

Function qemuMonitorJSONBuildCPUModelInfoToJSON builds and returns JSON of form {"model": {"name": "IvyBridge", "props": {}}} from qemuMonitorCPUModelInfo. Function qemuMonitorJSONBuildCPUModelInfoToJSON returns virJsonValuePtr on success and NULL on failure. --- src/qemu/qemu_monitor_json.c | 48 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 4368aaaa0..44c1b2f15 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5337,6 +5337,54 @@ qemuMonitorJSONParseCPUModelProperty(const char *key, return 0; }

+/* model_json: {"model": {"name": "IvyBridge", "props": {}}} + */ +static virJSONValuePtr +qemuMonitorJSONBuildCPUModelInfoToJSON(qemuMonitorCPUModelInfoPtr model) +{ + virJSONValuePtr cpu_props = NULL; + virJSONValuePtr model_json = NULL; + size_t i; + + if (!(cpu_props = virJSONValueNewObject())) + goto cleanup; + + for (i = 0; i < model->nprops; i++) { + qemuMonitorCPUPropertyPtr prop = model->props + i;

+ + switch (prop->type) { + case QEMU_MONITOR_CPU_PROPERTY_BOOLEAN: + if (virJSONValueObjectAppendBoolean(cpu_props, prop->name, prop->value.boolean) < 0)

+ goto cleanup; + break; + + case QEMU_MONITOR_CPU_PROPERTY_STRING: + if (virJSONValueObjectAppendString(cpu_props, prop->name, prop->value.string) < 0) + goto cleanup; + break; + + case QEMU_MONITOR_CPU_PROPERTY_NUMBER: + if (virJSONValueObjectAppendNumberLong(cpu_props, prop->name, prop->value.number) < 0) + goto cleanup; + break; + + case QEMU_MONITOR_CPU_PROPERTY_LAST: + default: + virReportEnumRangeError(qemuMonitorCPUPropertyPtr, prop->type); + goto cleanup; + } + } + + if (virJSONValueObjectCreate(&model_json, "s:name", model->name, + "a:props", &cpu_props, NULL) < 0) { + virJSONValueFree(cpu_props); + goto cleanup; + }

+ + cleanup: + return model_json; +} + /* model_json: {"model": {"name": "IvyBridge", "props": {}}} */ static qemuMonitorCPUModelInfoPtr

Function qemuMonitorGetCPUModelBaseline exposed to carry out a QMP query-cpu-model-baseline transaction with QEMU. QEMU determines a baseline CPU Model from two input CPU Models to complete the query-cpu-model-baseline transaction. --- src/qemu/qemu_monitor.c | 13 ++++++++++ src/qemu/qemu_monitor.h | 5 ++++ src/qemu/qemu_monitor_json.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 7 ++++++ 4 files changed, 82 insertions(+) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 7b647525b..c1098ff72 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -3874,6 +3874,19 @@ qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig) return NULL; }

+int +qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline) +{ + VIR_DEBUG("model_a->name=%s", model_a->name); + VIR_DEBUG("model_b->name=%s", model_b->name); + + QEMU_CHECK_MONITOR_JSON(mon); + + return qemuMonitorJSONGetCPUModelBaseline(mon, model_a, model_b, model_baseline); +}

int qemuMonitorGetCommands(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index d04148e56..c7a80ca63 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1079,6 +1079,11 @@ void qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); qemuMonitorCPUModelInfoPtr qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig); +int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline); + int qemuMonitorGetCommands(qemuMonitorPtr mon, char ***commands); int qemuMonitorGetEvents(qemuMonitorPtr mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 44c1b2f15..8cb10268f 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -5539,6 +5539,63 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, return ret; } +int +qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr reply = NULL; + virJSONValuePtr data = NULL; + virJSONValuePtr modela = NULL; + virJSONValuePtr modelb = NULL; + + *model_baseline = NULL; + + if (!(modela = qemuMonitorJSONBuildCPUModelInfoToJSON(model_a))) + goto cleanup; + + if (!(modelb = qemuMonitorJSONBuildCPUModelInfoToJSON(model_b))) + goto cleanup; + + if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-baseline", + "a:modela", &modela, + "a:modelb", &modelb, + NULL))) + goto cleanup; + + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) + goto cleanup; + + /* Urgh, some QEMU architectures have query-cpu-model-baseline + * command but return 'GenericError' with string "Not supported", + * instead of simply omitting the command entirely + */ + if (qemuMonitorJSONHasError(reply, "GenericError")) { + ret = 0;

+ goto cleanup; + } + + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) + goto cleanup; + + data = virJSONValueObjectGetObject(reply, "return");

+ + if (!(*model_baseline = qemuMonitorJSONBuildCPUModelInfoFromJSON(data))) + goto cleanup;

+ + ret = 0; + + cleanup: + virJSONValueFree(cmd); + virJSONValueFree(reply); + virJSONValueFree(modela); + virJSONValueFree(modelb);> + + return ret; +} int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index 045df4919..aa6f3582e 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -361,6 +361,13 @@ int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon, qemuMonitorCPUModelInfoPtr *model_info) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); +int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, + qemuMonitorCPUModelInfoPtr model_a, + qemuMonitorCPUModelInfoPtr model_b, + qemuMonitorCPUModelInfoPtr *model_baseline) + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + + int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, char ***commands) ATTRIBUTE_NONNULL(2);

Please see responses below. New patchset with all recommended changes will be sent soon. Chris ----- Original Message ----- > From: "John Ferlan" <jferlan(a)redhat.com&gt; > To: "Chris Venteicher" <cventeic(a)redhat.com&gt;, libvir-list(a)redhat.com > Sent: Friday, April 27, 2018 1:41:03 PM > Subject: Re: [libvirt] [PATCHv2 3/3] qemu_monitor: query-cpu-model-baseline QMP command > > > $SUBJ: > > qemu: Introduce qemuMonitorGetCPUModelBaseline > > > On 04/19/2018 12:06 AM, Chris Venteicher wrote: >> Function qemuMonitorGetCPUModelBaseline exposed to carry out a QMP >> query-cpu-model-baseline transaction with QEMU. >> >> QEMU determines a baseline CPU Model from two input CPU Models to >> complete the query-cpu-model-baseline transaction. >> --- >> src/qemu/qemu_monitor.c | 13 ++++++++++ >> src/qemu/qemu_monitor.h | 5 ++++ >> src/qemu/qemu_monitor_json.c | 57 >> ++++++++++++++++++++++++++++++++++++++++++++ >> src/qemu/qemu_monitor_json.h | 7 ++++++ >> 4 files changed, 82 insertions(+) >> >> diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c >> index 7b647525b..c1098ff72 100644 >> --- a/src/qemu/qemu_monitor.c >> +++ b/src/qemu/qemu_monitor.c >> @@ -3874,6 +3874,19 @@ qemuMonitorCPUModelInfoCopy(const >> qemuMonitorCPUModelInfo *orig) >> return NULL; >> } >> > > 2 blank lines. > >> +int >> +qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, >> + qemuMonitorCPUModelInfoPtr model_a, >> + qemuMonitorCPUModelInfoPtr model_b, >> + qemuMonitorCPUModelInfoPtr *model_baseline) >> +{ >> + VIR_DEBUG("model_a->name=%s", model_a->name); >> + VIR_DEBUG("model_b->name=%s", model_b->name); >> + >> + QEMU_CHECK_MONITOR_JSON(mon); >> + >> + return qemuMonitorJSONGetCPUModelBaseline(mon, model_a, model_b, >> model_baseline); >> +} > > No consumer for this yet - are there plans? Just remember that for the > consumer we'll probably need some sort of qemu capability to manage > whether or not the command "query-cpu-model-baseline" exists for the > particular version or not. > The consumer is in a future patch. Want to get some early feedback on these changes. >> >> int >> qemuMonitorGetCommands(qemuMonitorPtr mon, >> diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h >> index d04148e56..c7a80ca63 100644 >> --- a/src/qemu/qemu_monitor.h >> +++ b/src/qemu/qemu_monitor.h >> @@ -1079,6 +1079,11 @@ void >> qemuMonitorCPUModelInfoFree(qemuMonitorCPUModelInfoPtr model_info); >> qemuMonitorCPUModelInfoPtr >> qemuMonitorCPUModelInfoCopy(const qemuMonitorCPUModelInfo *orig); >> >> +int qemuMonitorGetCPUModelBaseline(qemuMonitorPtr mon, >> + qemuMonitorCPUModelInfoPtr model_a, >> + qemuMonitorCPUModelInfoPtr model_b, >> + qemuMonitorCPUModelInfoPtr >> *model_baseline); >> + >> int qemuMonitorGetCommands(qemuMonitorPtr mon, >> char ***commands); >> int qemuMonitorGetEvents(qemuMonitorPtr mon, >> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c >> index 44c1b2f15..8cb10268f 100644 >> --- a/src/qemu/qemu_monitor_json.c >> +++ b/src/qemu/qemu_monitor_json.c >> @@ -5539,6 +5539,63 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr >> mon, >> return ret; >> } >> >> +int >> +qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, >> + qemuMonitorCPUModelInfoPtr model_a, >> + qemuMonitorCPUModelInfoPtr model_b, >> + qemuMonitorCPUModelInfoPtr >> *model_baseline) >> +{ >> + int ret = -1; >> + virJSONValuePtr cmd = NULL; >> + virJSONValuePtr reply = NULL; >> + virJSONValuePtr data = NULL; >> + virJSONValuePtr modela = NULL; >> + virJSONValuePtr modelb = NULL; >> + >> + *model_baseline = NULL; >> + >> + if (!(modela = qemuMonitorJSONBuildCPUModelInfoToJSON(model_a))) >> + goto cleanup; >> + >> + if (!(modelb = qemuMonitorJSONBuildCPUModelInfoToJSON(model_b))) >> + goto cleanup; >> + >> + if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-baseline", >> + "a:modela", &modela, >> + "a:modelb", &modelb, >> + NULL))) >> + goto cleanup; >> + >> + if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0) >> + goto cleanup; >> + >> + /* Urgh, some QEMU architectures have query-cpu-model-baseline >> + * command but return 'GenericError' with string "Not supported", >> + * instead of simply omitting the command entirely >> + */ >> + if (qemuMonitorJSONHasError(reply, "GenericError")) { >> + ret = 0; > > So this won't be an error then? > > Or is this because you're calling without checking whether or not the > capability exists in the target emulator? A goal is to gracefully handle different levels of query-cpu-model-baseline support as it's only supported by S390 now but other archs will likely be added in the future. Some archs will likely never be supported. Seems like there are scenarios where we would fall back to code in libvirt to provide an answer in cases where query-cpu-model-baseline is not able to answer.

With that said, I am primarily following the pattern of checking for "GenericError" that has been established in the code for similar QMP requests. The check might not be needed but I am pretty sure it's not harmful. Please let me know if it's not making sense. For reference, I hacked qemu to generate responses on X86 for testing which involved making sure qemu did not unregister the command. Perhaps that might be useful info. chris@cv1:~/libvirt/qemu_git$ git diff monitor.c diff --git a/monitor.c b/monitor.c index a4417f26cd..760d6ada00 100644 --- a/monitor.c +++ b/monitor.c @@ -994,8 +994,8 @@ static void qmp_unregister_commands_hack(void) qmp_unregister_command(&qmp_commands, "query-cpu-model-expansion"); #endif #if !defined(TARGET_S390X) - qmp_unregister_command(&qmp_commands, "query-cpu-model-baseline"); - qmp_unregister_command(&qmp_commands, "query-cpu-model-comparison"); +// qmp_unregister_command(&qmp_commands, "query-cpu-model-baseline"); +// qmp_unregister_command(&qmp_commands, "query-cpu-model-comparison"); #endif #if !defined(TARGET_PPC) && !defined(TARGET_ARM) && !defined(TARGET_I386) \ && !defined(TARGET_S390X) > >> + goto cleanup; >> + } >> + >> + if (qemuMonitorJSONCheckReply(cmd, reply, VIR_JSON_TYPE_OBJECT) < 0) >> + goto cleanup; >> + >> + data = virJSONValueObjectGetObject(reply, "return"); > > Here's where I'd add the : > > if (!(cpu_model = virJSONValueObjectGetObject(data, "model"))) { > virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > _("query-cpu-model-baseline reply data was > missing 'model'")); > goto cleanup; > } > >> + >> + if (!(*model_baseline = >> qemuMonitorJSONBuildCPUModelInfoFromJSON(data))) >> + goto cleanup; > > and pass the cpu_model here > > > John > >> + >> + ret = 0; >> + >> + cleanup: >> + virJSONValueFree(cmd); >> + virJSONValueFree(reply); >> + virJSONValueFree(modela); >> + virJSONValueFree(modelb);> + >> + return ret; >> +} >> >> int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, >> char ***commands) >> diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h >> index 045df4919..aa6f3582e 100644 >> --- a/src/qemu/qemu_monitor_json.h >> +++ b/src/qemu/qemu_monitor_json.h >> @@ -361,6 +361,13 @@ int qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr >> mon, >> qemuMonitorCPUModelInfoPtr >> *model_info) >> ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(5); >> >> +int qemuMonitorJSONGetCPUModelBaseline(qemuMonitorPtr mon, >> + qemuMonitorCPUModelInfoPtr model_a, >> + qemuMonitorCPUModelInfoPtr model_b, >> + qemuMonitorCPUModelInfoPtr >> *model_baseline) >> + ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); >> + >> + >> int qemuMonitorJSONGetCommands(qemuMonitorPtr mon, >> char ***commands) >> ATTRIBUTE_NONNULL(2); >> > > -- > libvir-list mailing list > libvir-list(a)redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list >