On Thu, Oct 08, 2020 at 11:36:56AM -0500, Jonathon Jongsma wrote:
On Thu, 8 Oct 2020 16:26:56 +0200 Pavel Hrdina <phrdina@redhat.com> wrote:
As preparation for g_autoptr() we need to change the function to take only virCgroupPtr.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-lxc.c | 4 +- src/lxc/lxc_cgroup.c | 4 +- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/lxc/lxc_process.c | 11 +++--- src/qemu/qemu_cgroup.c | 12 +++--- src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 32 +++++++-------- src/qemu/qemu_process.c | 2 +- src/util/vircgroup.c | 65 +++++++++++++++++-------------- src/util/vircgroup.h | 2 +- src/util/vircgroupv1.c | 2 +- tests/vircgrouptest.c | 48 +++++++++++------------ tools/virt-host-validate-common.c | 2 +- 15 files changed, 102 insertions(+), 91 deletions(-)
diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c index 25f1cfc5f7..73daf123f0 100644 --- a/src/libvirt-lxc.c +++ b/src/libvirt-lxc.c @@ -307,12 +307,12 @@ int virDomainLxcEnterCGroup(virDomainPtr domain, if (virCgroupAddProcess(cgroup, getpid()) < 0) goto error;
- virCgroupFree(&cgroup); + virCgroupFree(cgroup);
return 0;
error: virDispatchError(NULL); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return -1; } diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index d13f2adde5..b80a8911f9 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -168,7 +168,7 @@ int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -417,7 +417,7 @@ virCgroupPtr virLXCCgroupCreate(virDomainDefPtr def, def->idmap.uidmap[0].target, def->idmap.gidmap[0].target, (1 << VIR_CGROUP_CONTROLLER_SYSTEMD)) < 0) { - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return NULL; } } diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index d1aa622be4..913f4de26a 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1668,7 +1668,7 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, ret = 0;
cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index f70731bc64..e6dee85ec7 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -310,7 +310,7 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) g_free(ctrl->nbdpids);
g_free(ctrl->nsFDs); - virCgroupFree(&ctrl->cgroup); + virCgroupFree(ctrl->cgroup);
/* This must always be the last thing to be closed */ VIR_FORCE_CLOSE(ctrl->handshakeFd); diff --git a/src/lxc/lxc_domain.c b/src/lxc/lxc_domain.c index d8aebe06d9..df60519fca 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -168,7 +168,7 @@ virLXCDomainObjPrivateFree(void *data) { virLXCDomainObjPrivatePtr priv = data;
- virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); virLXCDomainObjFreeJob(priv); g_free(priv); } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 16969dbf33..a98a090893 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -236,7 +236,8 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, if (priv->cgroup) { virCgroupRemove(priv->cgroup); - virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); + priv->cgroup = NULL; }
/* Get machined to terminate the machine as it may not have cleaned it @@ -1202,26 +1203,26 @@ int virLXCProcessStart(virConnectPtr conn, if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) { - virCgroupFree(&selfcgroup); + virCgroupFree(selfcgroup); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'cpuacct' cgroups controller mount")); return -1; } if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_DEVICES)) { - virCgroupFree(&selfcgroup); + virCgroupFree(selfcgroup); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'devices' cgroups controller mount")); return -1; } if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_MEMORY)) { - virCgroupFree(&selfcgroup); + virCgroupFree(selfcgroup); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'memory' cgroups controller mount")); return -1; } - virCgroupFree(&selfcgroup); + virCgroupFree(selfcgroup);
if (vm->def->nconsoles == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 9017753053..473ca8a414 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -921,7 +921,8 @@ qemuInitCgroup(virDomainObjPtr vm, if (!virCgroupAvailable()) return 0;
- virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); + priv->cgroup = NULL;
if (!vm->def->resource) { virDomainResourceDefPtr res; @@ -983,7 +984,7 @@ qemuRestoreCgroupThread(virCgroupPtr cgroup,
ret = 0; cleanup: - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); return ret; }
@@ -1054,7 +1055,8 @@ qemuConnectCgroup(virDomainObjPtr vm) if (!virCgroupAvailable()) return 0;
- virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); + priv->cgroup = NULL;
if (virCgroupNewDetectMachine(vm->def->name, "qemu", @@ -1150,7 +1152,7 @@ qemuSetupCgroupForExtDevices(virDomainObjPtr vm, ret = qemuExtDevicesSetupCgroup(driver, vm, cgroup_temp);
cleanup: - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp);
return ret; } @@ -1221,7 +1223,7 @@ qemuCgroupEmulatorAllNodesDataFree(qemuCgroupEmulatorAllNodesDataPtr data) if (!data) return;
- virCgroupFree(&data->emulatorCgroup); + virCgroupFree(data->emulatorCgroup); VIR_FREE(data->emulatorMemMask); VIR_FREE(data); } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9623123d3c..2d7f61f5e9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1725,7 +1725,8 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) g_strfreev(priv->qemuDevices); priv->qemuDevices = NULL;
- virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); + priv->cgroup = NULL;
virPerfFree(priv->perf); priv->perf = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8ef812cd94..60e043115f 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4583,7 +4583,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm,
cleanup: virBitmapFree(tmpmap); - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); virObjectEventStateQueue(driver->domainEventState, event); return ret; } @@ -4809,7 +4809,7 @@ qemuDomainPinEmulator(virDomainPtr dom,
cleanup: if (cgroup_emulator) - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); virObjectEventStateQueue(driver->domainEventState, event); virBitmapFree(pcpumap); virDomainObjEndAPI(&vm); @@ -5287,7 +5287,7 @@ qemuDomainPinIOThread(virDomainPtr dom,
cleanup: if (cgroup_iothread) - virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); virObjectEventStateQueue(driver->domainEventState, event); virBitmapFree(pcpumap); virDomainObjEndAPI(&vm); @@ -8717,7 +8717,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp);
for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); @@ -8729,7 +8729,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); }
for (i = 0; i < vm->def->niothreadids; i++) { @@ -8738,7 +8738,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); }
/* set nodeset for root cgroup */ @@ -8747,7 +8747,7 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm,
ret = 0; cleanup: - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp);
I realize that this particular issue only exists until you switch to using autoptr in patch 7, but you're technically introducing a bug here. virCgroupFree() no longer clears the ptr to NULL, so when you free cgroup_temp up above within this function, it is now a dangling pointer, and then we try to free it again in the cleanup section. Previously this cleanup call to virCgroupFree() was a no-op for all non-error paths.
There are several other examples within this patch.
Nice catch! I'll fix it before pushing. Pavel