On 10/04/2017 10:58 AM, Jiri Denemark wrote:
> query-cpu-definitions QMP command returns a list of unavailable features
> which prevent CPU models from being usable on the current host. So far
> we only checked whether the list was empty to mark CPU models as
> (un)usable. This patch parses all unavailable features for each CPU
> model and stores them in virDomainCapsCPUModel as a list of usability
> blockers.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> src/qemu/qemu_capabilities.c | 2 +-
> src/qemu/qemu_monitor.c | 2 +
> src/qemu/qemu_monitor.h | 1 +
> src/qemu/qemu_monitor_json.c | 26 +-
> tests/qemucapabilitiesdata/caps_2.10.0.s390x.xml | 1102 +++++++++++++++++++--
> tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 236 ++++-
> tests/qemucapabilitiesdata/caps_2.8.0.x86_64.xml | 154 ++-
> tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 154 ++-
> 8 files changed, 1556 insertions(+), 121 deletions(-)
>
[...]
> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> index c63d250d36..fcdd58b369 100644
> --- a/src/qemu/qemu_monitor_json.c
> +++ b/src/qemu/qemu_monitor_json.c
> @@ -5076,6 +5076,8 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
>
> if (virJSONValueObjectHasKey(child, "unavailable-features")) {
> virJSONValuePtr blockers;
> + size_t j;
> + int len;
>
> blockers = virJSONValueObjectGetArray(child,
> "unavailable-features");
> @@ -5086,10 +5088,32 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
> goto cleanup;
> }
>
> - if (virJSONValueArraySize(blockers) > 0)
> + len = virJSONValueArraySize(blockers);
> +
> + if (len != 0)
At this point len is either 0 (empty) or > 0 because if it was < 0 the
virJSONValueObjectGetArray would have already caused a failure, right?
So why not :
if (len == 0) {
cpu->usable = VIR_TRISTATE_BOOL_YES;
continue;
}
cpu->usable = VIR_TRISTATE_BOOL_NO;
if (VIR_ALLOC_N(cpu->blockers, len + 1) < 0)
...
> cpu->usable = VIR_TRISTATE_BOOL_NO;
> else
> cpu->usable = VIR_TRISTATE_BOOL_YES;
> +
> + if (len > 0 && VIR_ALLOC_N(cpu->blockers, len + 1) < 0)
> + goto cleanup;
> +
> + for (j = 0; j < len; j++) {
> + virJSONValuePtr blocker = virJSONValueArrayGet(blockers, j);
> + char *name;
virJSONValueArrayGet can return NULL, but that shouldn't affect us since
blockers is an ARRAY and there's no way j >= len...
> +
> + if (blocker->type != VIR_JSON_TYPE_STRING) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("unexpected value in unavailable-features "
> + "array"));
> + goto cleanup;
> + }
...but because of this check...
> +
> + if (VIR_STRDUP(name, virJSONValueGetString(blocker)) < 0)
...wouldn't virJSONValueGetString return a non NULL string, so...
> + goto cleanup;
> +
> + cpu->blockers[j] = name;
...why not just go direct to cpu->blockers[j]? Or did you come across
something in testing that had a NULL return from the call to
virJSONValueGetString(blocker)?
Maybe a NULL entry should just be ignored so as to not tank the whole
thing using the logic that if a blocker isn't there, by name, then is it
a blocker?
> + }
> }
> }
>
Reviewed-by: John Ferlan <jferlan@xxxxxxxxxx>
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
