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 <wall...@linux.ibm.com>
> Reviewed-by: Daniel Henrique Barboza <danielh...@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 <jdene...@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 "
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
