Re: [libvirt PATCH 03/10] util: vircgroup: change virCgroupFree to take only virCgroupPtr

On Thu, 8 Oct 2020 16:26:56 +0200 Pavel Hrdina <phrdina(a)redhat.com&gt; wrote: > As preparation for g_autoptr() we need to change the function to take > only virCgroupPtr. > > Signed-off-by: Pavel Hrdina <phrdina(a)redhat.com&gt; > --- > 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.