Re: [libvirt] [PATCHv2 3/3] qemu_monitor: query-cpu-model-baseline QMP command

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 >