Re: [libvirt] [PATCH v5 09/15] qemu_driver: hook up query-cpu-model-baseline

On Thu, Sep 19, 2019 at 16:25:00 -0400, Collin Walling wrote: 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. This would cause double free in the caller. Adding cleanup label here would be better. You can avoid this by using VIR_STEAL_PTR(info, *src) at the beginning. 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. Extra empty line. I think this comment would be better placed as a documentation of the new function. You could steal from baseline to ret, free baseline unconditionally and return ret to get rid of the error label. Three extra empty lines in this hunk. With the following patch squashed in Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt; 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 "