On Thu, Sep 19, 2019 at 16:25:00 -0400, Collin Walling wrote:
This command is hooked into the virsh hypervisor-cpu-baseline command. The CPU models provided in the XML sent to the command will be baselined via the query-cpu-model-baseline QMP command. The resulting CPU model will be reported.
Signed-off-by: Collin Walling <walling@linux.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielh413@gmail.com> --- src/qemu/qemu_driver.c | 88 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 1e041a8..2a5a3ca 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13810,6 +13810,83 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, }
+static int +qemuConnectStealCPUModelFromInfo(virCPUDefPtr dst, + qemuMonitorCPUModelInfoPtr *src) +{ + qemuMonitorCPUModelInfoPtr info = *src; + size_t i; + int ret = 0;
We usually initialize ret to -1 and set it to zero at the very end when everything is done rather than changing it to -1 on each error path.
+ + virCPUDefFreeModel(dst); + + VIR_STEAL_PTR(dst->model, info->name); + + for (i = 0; i < info->nprops; i++) { + char *name = info->props[i].name; + + if (info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN && + info->props[i].value.boolean && + virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) { + virCPUDefFree(dst);
This would cause double free in the caller.
+ ret = -1; + break; + } + } +
Adding cleanup label here would be better.
+ qemuMonitorCPUModelInfoFree(info); + *src = NULL;
You can avoid this by using VIR_STEAL_PTR(info, *src) at the beginning.
+ return ret; +} + + +static virCPUDefPtr +qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, + const char *libDir, + uid_t runUid, + gid_t runGid, + int ncpus, + virCPUDefPtr *cpus)
I think I mentioned in my previous review (probably not in this exact context, though) we usually pass an array followed by the number of elements.
+{ + qemuProcessQMPPtr proc; + virCPUDefPtr baseline = NULL; + qemuMonitorCPUModelInfoPtr result = NULL; + size_t i; + + if (!(proc = qemuProcessQMPNew(virQEMUCapsGetBinary(qemuCaps), + libDir, runUid, runGid, false))) + goto cleanup; + + if (qemuProcessQMPStart(proc) < 0) + goto cleanup; + + if (VIR_ALLOC(baseline) < 0) + goto error; + + if (virCPUDefCopyModel(baseline, cpus[0], false)) + goto error; + + for (i = 1; i < ncpus; i++) { +
Extra empty line.
+ if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, + cpus[i], &result) < 0) + goto error; + + /* result is freed regardless of this function's success */
I think this comment would be better placed as a documentation of the new function.
+ if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) + goto error; + } +
You could steal from baseline to ret, free baseline unconditionally and return ret to get rid of the error label.
+ cleanup: + qemuProcessQMPFree(proc); + return baseline; + + error: + virCPUDefFree(baseline); + goto cleanup; +} + + static char * qemuConnectBaselineHypervisorCPU(virConnectPtr conn, const char *emulator, @@ -13821,6 +13898,7 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, unsigned int flags) { virQEMUDriverPtr driver = conn->privateData; + VIR_AUTOUNREF(virQEMUDriverConfigPtr) cfg = virQEMUDriverGetConfig(driver); virCPUDefPtr *cpus = NULL; virQEMUCapsPtr qemuCaps = NULL; virArch arch; @@ -13875,6 +13953,16 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, (const char **)features, migratable))) goto cleanup; + + } else if (ARCH_IS_S390(arch) && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) { + + if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, + cfg->user, cfg->group, + ncpus, + cpus))) + goto cleanup; +
Three extra empty lines in this hunk.
} else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported "
With the following patch squashed in Reviewed-by: Jiri Denemark <jdenemar@redhat.com> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index b25e554358..b772a920e3 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -13739,32 +13739,41 @@ qemuConnectBaselineCPU(virConnectPtr conn ATTRIBUTE_UNUSED, } +/** + * qemuConnectStealCPUModelFromInfo: + * + * Consumes @src and replaces the content of @dst with CPU model name and + * features from @src. When this function returns (both with success or + * failure), @src is freed. + */ static int qemuConnectStealCPUModelFromInfo(virCPUDefPtr dst, qemuMonitorCPUModelInfoPtr *src) { - qemuMonitorCPUModelInfoPtr info = *src; + qemuMonitorCPUModelInfoPtr info; size_t i; - int ret = 0; + int ret = -1; virCPUDefFreeModel(dst); + VIR_STEAL_PTR(info, *src); VIR_STEAL_PTR(dst->model, info->name); for (i = 0; i < info->nprops; i++) { char *name = info->props[i].name; - if (info->props[i].type == QEMU_MONITOR_CPU_PROPERTY_BOOLEAN && - info->props[i].value.boolean && - virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) { - virCPUDefFree(dst); - ret = -1; - break; - } + if (info->props[i].type != QEMU_MONITOR_CPU_PROPERTY_BOOLEAN || + !info->props[i].value.boolean) + continue; + + if (virCPUDefAddFeature(dst, name, VIR_CPU_FEATURE_REQUIRE) < 0) + goto cleanup; } + ret = 0; + + cleanup: qemuMonitorCPUModelInfoFree(info); - *src = NULL; return ret; } @@ -13774,10 +13783,11 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, const char *libDir, uid_t runUid, gid_t runGid, - int ncpus, - virCPUDefPtr *cpus) + virCPUDefPtr *cpus, + int ncpus) { qemuProcessQMPPtr proc; + virCPUDefPtr ret = NULL; virCPUDefPtr baseline = NULL; qemuMonitorCPUModelInfoPtr result = NULL; size_t i; @@ -13790,29 +13800,26 @@ qemuConnectCPUModelBaseline(virQEMUCapsPtr qemuCaps, goto cleanup; if (VIR_ALLOC(baseline) < 0) - goto error; + goto cleanup; if (virCPUDefCopyModel(baseline, cpus[0], false)) - goto error; + goto cleanup; for (i = 1; i < ncpus; i++) { - if (qemuMonitorGetCPUModelBaseline(proc->mon, baseline, cpus[i], &result) < 0) - goto error; + goto cleanup; - /* result is freed regardless of this function's success */ if (qemuConnectStealCPUModelFromInfo(baseline, &result) < 0) - goto error; + goto cleanup; } + VIR_STEAL_PTR(ret, baseline); + cleanup: qemuProcessQMPFree(proc); - return baseline; - - error: virCPUDefFree(baseline); - goto cleanup; + return ret; } @@ -13882,16 +13889,12 @@ qemuConnectBaselineHypervisorCPU(virConnectPtr conn, if (!(cpu = virCPUBaseline(arch, cpus, ncpus, cpuModels, (const char **)features, migratable))) goto cleanup; - } else if (ARCH_IS_S390(arch) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_QUERY_CPU_MODEL_BASELINE)) { - if (!(cpu = qemuConnectCPUModelBaseline(qemuCaps, cfg->libDir, cfg->user, cfg->group, - ncpus, - cpus))) + cpus, ncpus))) goto cleanup; - } else { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, _("computing baseline hypervisor CPU is not supported "