On 04/30/2018 01:25 PM, Chris Venteicher wrote:
> Please see responses below.
>
> New patchset with all recommended changes will be sent soon.
>
> Chris
>
> ----- Original Message -----
>> From: "John Ferlan" <jferlan@xxxxxxxxxx>
>> To: "Chris Venteicher" <cventeic@xxxxxxxxxx>, libvir-list@xxxxxxxxxx
>> Sent: Friday, April 27, 2018 1:41:03 PM
>> Subject: Re: [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.
>
Please be sure to add comments prior to the function to describe the
input, output, and return values. One thing we've done with some
commands which need to fallback to some other command is use different
error codes so the caller makes the determination... Sometimes it's 1,
0, -1, while others it's 0, -1, -2
Perhaps it's just a matter of describing expectations of usage. If the
caller sees a 0 return, then it would also have to check if
*model_baseline != NULL in order to make an intelligent decision. Or it
could just use the return value and go from there.
Your call, since the consumer wasn't posted - I wanted to be sure to ask
and make it a known...
John
>
> 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@xxxxxxxxxx
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
