Re: [libvirt] [PATCH v2 1/8] qemu_monitor: helper functions for CPU models
On 4/29/19 10:08 AM, Daniel Henrique Barboza wrote:
On 4/26/19 5:22 PM, walling(a)linux.ibm.com wrote:
> From: Collin Walling<walling(a)linux.ibm.com>
>
> Refactor some code in qemuMonitorJSONGetCPUModelExpansion to be
> later used for the comparison and baseline functions.
>
> This does not alter any functionality.
>
> Signed-off-by: Collin Walling<walling(a)linux.ibm.com>
> Reviewed-by: Bjoern Walk<bwalk(a)linux.ibm.com>
> ---
> src/qemu/qemu_monitor_json.c | 170
> ++++++++++++++++++++++++++++++-------------
> 1 file changed, 118 insertions(+), 52 deletions(-)
>
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index 908967f..9618d8e 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5513,6 +5513,118 @@ qemuMonitorJSONParseCPUModelProperty(const
> char *key,
> return 0;
> }
> +
> +static virJSONValuePtr
> +qemuMonitorJSONMakeCPUModel(const char *model_name,
> + size_t nprops,
> + virCPUFeatureDefPtr props,
> + bool migratable)
> +{
> + virJSONValuePtr value;
> + virJSONValuePtr feats = NULL;
> + size_t i;
> +
> + if (!(value = virJSONValueNewObject()))
> + goto cleanup;
> +
> + if (virJSONValueObjectAppendString(value, "name", model_name) < 0)
> + goto cleanup;
> +
> + if (nprops || !migratable) {
> + if (!(feats = virJSONValueNewObject()))
> + goto cleanup;
> +
> + for (i = 0; i < nprops; i++) {
> + char *name = props[i].name;
> + bool enabled = false;
> +
> + if (props[i].policy == VIR_CPU_FEATURE_REQUIRE ||
> + props[i].policy == VIR_CPU_FEATURE_FORCE)
> + enabled = true;
> +
> + if (virJSONValueObjectAppendBoolean(feats, name, enabled)
> < 0)
> + goto cleanup;
> + }
> +
> + if (!migratable) {
> + if (virJSONValueObjectAppendBoolean(feats, "migratable",
> false) < 0)
> + goto cleanup;
Missing indentation of 'goto cleanup' after the if clause.
Good eye, thanks!
> + }
> +
> + if (virJSONValueObjectAppend(value, "props", feats) < 0)
> + goto cleanup;
> + }
> +
> + return value;
> +
> + cleanup:
> + virJSONValueFree(value);
> + virJSONValueFree(feats);
> + return NULL;
> +}
> +
[...]
> -
> - if (!(cpu_props = virJSONValueObjectGetObject(cpu_model,
> "props"))) {
> - virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> - _("query-cpu-model-expansion reply data was
> missing 'props'"));
> - goto cleanup;
> - }
These error messages 'query-cpu-model-expansion reply data was
missing ...' were replaced inside qemuMonitorJSONParseCPUModelData
by error messages of the type 'QMP command reply data was missing ...'.
I see that this was done because the function is used in later patches
with different QMP commands, hence this change to a more generic
error message. Which is fine.
A suggestion here would be to pass an extra string 'qmp-command-name'
to qemuMonitorJSONParseCPUModelData. Then these error messages
can be tuned to show which QMP command failed to parse. This can be
useful if this parse function is used multiple times with several QMP
commands.
Thanks,
DHB
Fair enough. This is an easy change.
Thanks for all of the review!
[...]