[libvirt PATCH 00/10] use g_autoptr with cgroups (glib chronicles)
Pavel Hrdina (10): qemu_cgroup: introduce qemuRestoreCgroupThread helper util: vircgroup: use GLib alloc functions util: vircgroup: change virCgroupFree to take only virCgroupPtr util: vircgroup: introduce g_autoptr() for virCgroup libvirt-lxc: use g_autoptr for virCgroup lxc: use g_autoptr for virCgroup qemu: use g_autoptr for virCgroup util: use g_autoptr for virCgroup tests: use g_autoptr for virCgroup tools: use g_autoptr for virCgroup src/libvirt-lxc.c | 5 +- src/lxc/lxc_cgroup.c | 20 +-- src/lxc/lxc_container.c | 37 ++--- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/lxc/lxc_process.c | 9 +- src/qemu/qemu_cgroup.c | 87 ++++++----- src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 139 ++++++------------ src/qemu/qemu_process.c | 2 +- src/util/vircgroup.c | 198 +++++++++++-------------- src/util/vircgroup.h | 4 +- src/util/vircgroupv1.c | 9 +- tests/vircgrouptest.c | 236 +++++++++++------------------- tools/virt-host-validate-common.c | 4 +- 15 files changed, 305 insertions(+), 452 deletions(-) -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 71 +++++++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index b671991ad6..9017753053 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -960,16 +960,41 @@ qemuInitCgroup(virDomainObjPtr vm, return 0; } -static void -qemuRestoreCgroupState(virDomainObjPtr vm) +static int +qemuRestoreCgroupThread(virCgroupPtr cgroup, + virCgroupThreadName thread, + int id) { - g_autofree char *mem_mask = NULL; + virCgroupPtr cgroup_temp = NULL; g_autofree char *nodeset = NULL; + int ret = -1; + + if (virCgroupNewThread(cgroup, thread, id, false, &cgroup_temp) < 0) + goto cleanup; + + if (virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0) + goto cleanup; + + if (virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0) + goto cleanup; + + if (virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0) + goto cleanup; + + ret = 0; + cleanup: + virCgroupFree(&cgroup_temp); + return ret; +} + +static void +qemuRestoreCgroupState(virDomainObjPtr vm) +{ + g_autofree char *mem_mask = NULL; int empty = -1; qemuDomainObjPrivatePtr priv = vm->privateData; size_t i = 0; g_autoptr(virBitmap) all_nodes = NULL; - virCgroupPtr cgroup_temp = NULL; if (!virNumaIsAvailable() || !virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET)) @@ -994,45 +1019,27 @@ qemuRestoreCgroupState(virDomainObjPtr vm) if (!vcpu->online) continue; - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i, - false, &cgroup_temp) < 0 || - virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0 || - virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0 || - virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0) - goto cleanup; - - VIR_FREE(nodeset); - virCgroupFree(&cgroup_temp); + if (qemuRestoreCgroupThread(priv->cgroup, + VIR_CGROUP_THREAD_VCPU, i) < 0) + return; } for (i = 0; i < vm->def->niothreadids; i++) { - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, - vm->def->iothreadids[i]->iothread_id, - false, &cgroup_temp) < 0 || - virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0 || - virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0 || - virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0) - goto cleanup; - - VIR_FREE(nodeset); - virCgroupFree(&cgroup_temp); + if (qemuRestoreCgroupThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, + vm->def->iothreadids[i]->iothread_id) < 0) + return; } - if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, - false, &cgroup_temp) < 0 || - virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0 || - virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0 || - virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0) - goto cleanup; + if (qemuRestoreCgroupThread(priv->cgroup, + VIR_CGROUP_THREAD_EMULATOR, 0) < 0) + return; - cleanup: - virCgroupFree(&cgroup_temp); return; error: virResetLastError(); VIR_DEBUG("Couldn't restore cgroups to meaningful state"); - goto cleanup; + return; } int -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 5c0543c66a..d52c0305f4 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -785,10 +785,8 @@ virCgroupSetPartitionSuffix(const char *path, char **res) */ if (STRNEQ(tokens[i], "") && !strchr(tokens[i], '.')) { - if (VIR_REALLOC_N(tokens[i], - strlen(tokens[i]) + strlen(".partition") + 1) < 0) - goto cleanup; - strcat(tokens[i], ".partition"); + g_autofree char *oldtoken = tokens[i]; + tokens[i] = g_strdup_printf("%s.partition", oldtoken); } if (virCgroupPartitionEscape(&(tokens[i])) < 0) -- 2.26.2
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); return ret; } @@ -9164,13 +9164,13 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) goto cleanup; - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); } return 0; cleanup: - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); return -1; } @@ -9191,11 +9191,11 @@ qemuSetEmulatorBandwidthLive(virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_emulator, period, quota) < 0) goto cleanup; - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); return 0; cleanup: - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); return -1; } @@ -9222,13 +9222,13 @@ qemuSetIOThreadsBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0) goto cleanup; - virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); } return 0; cleanup: - virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); return -1; } @@ -9601,7 +9601,7 @@ qemuGetVcpusBWLive(virDomainObjPtr vm, ret = 0; cleanup: - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); return ret; } @@ -9624,7 +9624,7 @@ qemuGetEmulatorBandwidthLive(virCgroupPtr cgroup, ret = 0; cleanup: - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); return ret; } @@ -9660,7 +9660,7 @@ qemuGetIOThreadsBWLive(virDomainObjPtr vm, ret = 0; cleanup: - virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); return ret; } diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6b5de29fdb..5bc76a75e3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2750,7 +2750,7 @@ qemuProcessSetupPid(virDomainObjPtr vm, if (cgroup) { if (ret < 0) virCgroupRemove(cgroup); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); } return ret; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index d52c0305f4..04876c6596 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -680,7 +680,7 @@ virCgroupNew(pid_t pid, return 0; error: - virCgroupFree(group); + virCgroupFree(*group); *group = NULL; return -1; @@ -860,9 +860,11 @@ virCgroupNewPartition(const char *path, ret = 0; cleanup: - if (ret != 0) - virCgroupFree(group); - virCgroupFree(&parent); + if (ret != 0) { + virCgroupFree(*group); + *group = NULL; + } + virCgroupFree(parent); return ret; } @@ -923,7 +925,8 @@ virCgroupNewDomainPartition(virCgroupPtr partition, */ if (virCgroupMakeGroup(partition, *group, create, VIR_CGROUP_MEM_HIERACHY) < 0) { - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; return -1; } @@ -976,7 +979,8 @@ virCgroupNewThread(virCgroupPtr domain, return -1; if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_THREAD) < 0) { - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; return -1; } @@ -1019,7 +1023,8 @@ virCgroupNewDetectMachine(const char *name, machinename)) { VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'", name, drivername); - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; return 0; } } @@ -1059,13 +1064,13 @@ virCgroupEnableMissingControllers(char *path, goto cleanup; if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_SYSTEMD) < 0) { - virCgroupFree(&tmp); + virCgroupFree(tmp); goto cleanup; } if (t) { *t = '/'; offset = t; - virCgroupFree(&parent); + virCgroupFree(parent); parent = tmp; } else { *group = tmp; @@ -1075,7 +1080,7 @@ virCgroupEnableMissingControllers(char *path, ret = 0; cleanup: - virCgroupFree(&parent); + virCgroupFree(parent); return ret; } @@ -1130,7 +1135,7 @@ virCgroupNewMachineSystemd(const char *name, break; } } - virCgroupFree(&init); + virCgroupFree(init); if (!path || STREQ(path, "/") || path[0] != '/') { VIR_DEBUG("Systemd didn't setup its controller, path=%s", @@ -1148,7 +1153,8 @@ virCgroupNewMachineSystemd(const char *name, virErrorPreserveLast(&saved); virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; virErrorRestore(&saved); } @@ -1199,7 +1205,8 @@ virCgroupNewMachineManual(const char *name, virErrorPreserveLast(&saved); virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; virErrorRestore(&saved); } @@ -1207,7 +1214,7 @@ virCgroupNewMachineManual(const char *name, ret = 0; cleanup: - virCgroupFree(&parent); + virCgroupFree(parent); return ret; } @@ -2059,12 +2066,12 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, sum_cpu_time[j] += tmp; } - virCgroupFree(&group_vcpu); + virCgroupFree(group_vcpu); } ret = 0; cleanup: - virCgroupFree(&group_vcpu); + virCgroupFree(group_vcpu); return ret; } @@ -2556,7 +2563,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if (dormdir) virCgroupRemove(subgroup); - virCgroupFree(&subgroup); + virCgroupFree(subgroup); } if (direrr < 0) goto cleanup; @@ -2565,7 +2572,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0; cleanup: - virCgroupFree(&subgroup); + virCgroupFree(subgroup); VIR_DIR_CLOSE(dp); return ret; } @@ -2767,7 +2774,7 @@ virCgroupControllerAvailable(int controller) return ret; ret = virCgroupHasController(cgroup, controller); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -3534,24 +3541,24 @@ virCgroupControllerAvailable(int controller G_GNUC_UNUSED) * @group: The group structure to free */ void -virCgroupFree(virCgroupPtr *group) +virCgroupFree(virCgroupPtr group) { size_t i; - if (*group == NULL) + if (group == NULL) return; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - VIR_FREE((*group)->legacy[i].mountPoint); - VIR_FREE((*group)->legacy[i].linkPoint); - VIR_FREE((*group)->legacy[i].placement); + VIR_FREE(group->legacy[i].mountPoint); + VIR_FREE(group->legacy[i].linkPoint); + VIR_FREE(group->legacy[i].placement); } - VIR_FREE((*group)->unified.mountPoint); - VIR_FREE((*group)->unified.placement); + VIR_FREE(group->unified.mountPoint); + VIR_FREE(group->unified.placement); - VIR_FREE((*group)->path); - VIR_FREE(*group); + VIR_FREE(group->path); + VIR_FREE(group); } @@ -3568,7 +3575,7 @@ virCgroupDelThread(virCgroupPtr cgroup, /* Remove the offlined cgroup */ virCgroupRemove(new_cgroup); - virCgroupFree(&new_cgroup); + virCgroupFree(new_cgroup); } return 0; diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 1dcd0688f1..691bec610f 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -107,7 +107,7 @@ int virCgroupTerminateMachine(const char *name) bool virCgroupNewIgnoreError(void); -void virCgroupFree(virCgroupPtr *group); +void virCgroupFree(virCgroupPtr group); bool virCgroupHasController(virCgroupPtr cgroup, int controller); int virCgroupPathOfController(virCgroupPtr group, diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 5504441fa6..52d2a17d39 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1484,7 +1484,7 @@ virCgroupV1MemoryOnceInit(void) "memory.limit_in_bytes", &mem_unlimited)); cleanup: - virCgroupFree(&group); + virCgroupFree(group); virCgroupV1MemoryUnlimitedKB = mem_unlimited >> 10; } diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index e5be556104..632d33efcd 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -217,7 +217,7 @@ testCgroupDetectMounts(const void *args) cleanup: g_unsetenv("VIR_CGROUP_MOCK_FILENAME"); VIR_FREE(parsed); - virCgroupFree(&group); + virCgroupFree(group); return result; } @@ -245,7 +245,7 @@ static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "", mountsFull, links, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -322,7 +322,7 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) goto cleanup; } ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall, NULL, NULL, 0); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); @@ -331,7 +331,7 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -371,7 +371,7 @@ static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) } /* Should now work */ - virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/deployment/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /deployment/production cgroup: %d\n", -rv); goto cleanup; @@ -381,7 +381,7 @@ static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) mountsFull, links, placementFull, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -420,14 +420,14 @@ static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) goto cleanup; } - virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user cgroup: %d\n", -rv); goto cleanup; } /* Should now work */ - virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user/production cgroup: %d\n", -rv); goto cleanup; @@ -437,7 +437,7 @@ static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) mountsFull, links, placementFull, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -473,8 +473,8 @@ static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) ret = validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&partitioncgroup); - virCgroupFree(&domaincgroup); + virCgroupFree(partitioncgroup); + virCgroupFree(domaincgroup); return ret; } @@ -524,10 +524,10 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED ret = validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&partitioncgroup3); - virCgroupFree(&partitioncgroup2); - virCgroupFree(&partitioncgroup1); - virCgroupFree(&domaincgroup); + virCgroupFree(partitioncgroup3); + virCgroupFree(partitioncgroup2); + virCgroupFree(partitioncgroup1); + virCgroupFree(domaincgroup); return ret; } @@ -553,7 +553,7 @@ static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -564,7 +564,7 @@ static int testCgroupNewForSelfLogind(const void *args G_GNUC_UNUSED) if (virCgroupNewSelf(&cgroup) >= 0) { fprintf(stderr, "Expected to fail, only systemd cgroup available.\n"); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return -1; } @@ -592,7 +592,7 @@ static int testCgroupNewForSelfUnified(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "", empty, empty, empty, "/not/really/sys/fs/cgroup", "/", controllers); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -630,7 +630,7 @@ static int testCgroupNewForSelfHybrid(const void *args G_GNUC_UNUSED) "/not/really/sys/fs/cgroup/unified", "/", controllers); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -756,7 +756,7 @@ static int testCgroupGetPercpuStats(const void *args G_GNUC_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); VIR_FREE(params); return ret; } @@ -789,7 +789,7 @@ static int testCgroupGetMemoryUsage(const void *args G_GNUC_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -849,7 +849,7 @@ testCgroupGetMemoryStat(const void *args G_GNUC_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -900,7 +900,7 @@ static int testCgroupGetBlkioIoServiced(const void *args G_GNUC_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -973,7 +973,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index c3784bb91d..4d8ec38f88 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -315,7 +315,7 @@ int virHostValidateCGroupControllers(const char *hvname, } } - virCgroupFree(&group); + virCgroupFree(group); return ret; } -- 2.26.2
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.
return ret; } @@ -9164,13 +9164,13 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) goto cleanup;
- virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); }
return 0;
cleanup: - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); return -1; }
@@ -9191,11 +9191,11 @@ qemuSetEmulatorBandwidthLive(virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_emulator, period, quota) < 0) goto cleanup;
- virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); return 0;
cleanup: - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); return -1; }
@@ -9222,13 +9222,13 @@ qemuSetIOThreadsBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0) goto cleanup;
- virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); }
return 0;
cleanup: - virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); return -1; }
@@ -9601,7 +9601,7 @@ qemuGetVcpusBWLive(virDomainObjPtr vm, ret = 0;
cleanup: - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); return ret; }
@@ -9624,7 +9624,7 @@ qemuGetEmulatorBandwidthLive(virCgroupPtr cgroup, ret = 0;
cleanup: - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); return ret; }
@@ -9660,7 +9660,7 @@ qemuGetIOThreadsBWLive(virDomainObjPtr vm, ret = 0;
cleanup: - virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); return ret; }
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6b5de29fdb..5bc76a75e3 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2750,7 +2750,7 @@ qemuProcessSetupPid(virDomainObjPtr vm, if (cgroup) { if (ret < 0) virCgroupRemove(cgroup); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); }
return ret; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index d52c0305f4..04876c6596 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -680,7 +680,7 @@ virCgroupNew(pid_t pid, return 0;
error: - virCgroupFree(group); + virCgroupFree(*group); *group = NULL;
return -1; @@ -860,9 +860,11 @@ virCgroupNewPartition(const char *path,
ret = 0; cleanup: - if (ret != 0) - virCgroupFree(group); - virCgroupFree(&parent); + if (ret != 0) { + virCgroupFree(*group); + *group = NULL; + } + virCgroupFree(parent); return ret; }
@@ -923,7 +925,8 @@ virCgroupNewDomainPartition(virCgroupPtr partition, */ if (virCgroupMakeGroup(partition, *group, create, VIR_CGROUP_MEM_HIERACHY) < 0) { - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; return -1; }
@@ -976,7 +979,8 @@ virCgroupNewThread(virCgroupPtr domain, return -1;
if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_THREAD) < 0) { - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; return -1; }
@@ -1019,7 +1023,8 @@ virCgroupNewDetectMachine(const char *name, machinename)) { VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'", name, drivername); - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; return 0; } } @@ -1059,13 +1064,13 @@ virCgroupEnableMissingControllers(char *path, goto cleanup;
if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_SYSTEMD) < 0) { - virCgroupFree(&tmp); + virCgroupFree(tmp); goto cleanup; } if (t) { *t = '/'; offset = t; - virCgroupFree(&parent); + virCgroupFree(parent); parent = tmp; } else { *group = tmp; @@ -1075,7 +1080,7 @@ virCgroupEnableMissingControllers(char *path,
ret = 0; cleanup: - virCgroupFree(&parent); + virCgroupFree(parent); return ret; }
@@ -1130,7 +1135,7 @@ virCgroupNewMachineSystemd(const char *name, break; } } - virCgroupFree(&init); + virCgroupFree(init);
if (!path || STREQ(path, "/") || path[0] != '/') { VIR_DEBUG("Systemd didn't setup its controller, path=%s", @@ -1148,7 +1153,8 @@ virCgroupNewMachineSystemd(const char *name,
virErrorPreserveLast(&saved); virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; virErrorRestore(&saved); }
@@ -1199,7 +1205,8 @@ virCgroupNewMachineManual(const char *name,
virErrorPreserveLast(&saved); virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); + *group = NULL; virErrorRestore(&saved); }
@@ -1207,7 +1214,7 @@ virCgroupNewMachineManual(const char *name, ret = 0;
cleanup: - virCgroupFree(&parent); + virCgroupFree(parent); return ret; }
@@ -2059,12 +2066,12 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, sum_cpu_time[j] += tmp; }
- virCgroupFree(&group_vcpu); + virCgroupFree(group_vcpu); }
ret = 0; cleanup: - virCgroupFree(&group_vcpu); + virCgroupFree(group_vcpu); return ret; }
@@ -2556,7 +2563,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if (dormdir) virCgroupRemove(subgroup);
- virCgroupFree(&subgroup); + virCgroupFree(subgroup); } if (direrr < 0) goto cleanup; @@ -2565,7 +2572,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0;
cleanup: - virCgroupFree(&subgroup); + virCgroupFree(subgroup); VIR_DIR_CLOSE(dp); return ret; } @@ -2767,7 +2774,7 @@ virCgroupControllerAvailable(int controller) return ret;
ret = virCgroupHasController(cgroup, controller); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -3534,24 +3541,24 @@ virCgroupControllerAvailable(int controller G_GNUC_UNUSED) * @group: The group structure to free */ void -virCgroupFree(virCgroupPtr *group) +virCgroupFree(virCgroupPtr group) { size_t i;
- if (*group == NULL) + if (group == NULL) return;
for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - VIR_FREE((*group)->legacy[i].mountPoint); - VIR_FREE((*group)->legacy[i].linkPoint); - VIR_FREE((*group)->legacy[i].placement); + VIR_FREE(group->legacy[i].mountPoint); + VIR_FREE(group->legacy[i].linkPoint); + VIR_FREE(group->legacy[i].placement); }
- VIR_FREE((*group)->unified.mountPoint); - VIR_FREE((*group)->unified.placement); + VIR_FREE(group->unified.mountPoint); + VIR_FREE(group->unified.placement);
- VIR_FREE((*group)->path); - VIR_FREE(*group); + VIR_FREE(group->path); + VIR_FREE(group); }
@@ -3568,7 +3575,7 @@ virCgroupDelThread(virCgroupPtr cgroup,
/* Remove the offlined cgroup */ virCgroupRemove(new_cgroup); - virCgroupFree(&new_cgroup); + virCgroupFree(new_cgroup); }
return 0; diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 1dcd0688f1..691bec610f 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -107,7 +107,7 @@ int virCgroupTerminateMachine(const char *name)
bool virCgroupNewIgnoreError(void);
-void virCgroupFree(virCgroupPtr *group); +void virCgroupFree(virCgroupPtr group);
bool virCgroupHasController(virCgroupPtr cgroup, int controller); int virCgroupPathOfController(virCgroupPtr group, diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 5504441fa6..52d2a17d39 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1484,7 +1484,7 @@ virCgroupV1MemoryOnceInit(void) "memory.limit_in_bytes", &mem_unlimited)); cleanup: - virCgroupFree(&group); + virCgroupFree(group); virCgroupV1MemoryUnlimitedKB = mem_unlimited >> 10; }
diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index e5be556104..632d33efcd 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -217,7 +217,7 @@ testCgroupDetectMounts(const void *args) cleanup: g_unsetenv("VIR_CGROUP_MOCK_FILENAME"); VIR_FREE(parsed); - virCgroupFree(&group); + virCgroupFree(group); return result; }
@@ -245,7 +245,7 @@ static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "", mountsFull, links, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -322,7 +322,7 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) goto cleanup; } ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall, NULL, NULL, 0); - virCgroupFree(&cgroup); + virCgroupFree(cgroup);
if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); @@ -331,7 +331,7 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -371,7 +371,7 @@ static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) }
/* Should now work */ - virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/deployment/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /deployment/production cgroup: %d\n", -rv); goto cleanup; @@ -381,7 +381,7 @@ static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) mountsFull, links, placementFull, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -420,14 +420,14 @@ static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) goto cleanup; }
- virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user cgroup: %d\n", -rv); goto cleanup; }
/* Should now work */ - virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user/production cgroup: %d\n", -rv); goto cleanup; @@ -437,7 +437,7 @@ static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) mountsFull, links, placementFull, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -473,8 +473,8 @@ static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) ret = validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&partitioncgroup); - virCgroupFree(&domaincgroup); + virCgroupFree(partitioncgroup); + virCgroupFree(domaincgroup); return ret; }
@@ -524,10 +524,10 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED ret = validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&partitioncgroup3); - virCgroupFree(&partitioncgroup2); - virCgroupFree(&partitioncgroup1); - virCgroupFree(&domaincgroup); + virCgroupFree(partitioncgroup3); + virCgroupFree(partitioncgroup2); + virCgroupFree(partitioncgroup1); + virCgroupFree(domaincgroup); return ret; }
@@ -553,7 +553,7 @@ static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -564,7 +564,7 @@ static int testCgroupNewForSelfLogind(const void *args G_GNUC_UNUSED) if (virCgroupNewSelf(&cgroup) >= 0) { fprintf(stderr, "Expected to fail, only systemd cgroup available.\n"); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return -1; }
@@ -592,7 +592,7 @@ static int testCgroupNewForSelfUnified(const void *args G_GNUC_UNUSED) ret = validateCgroup(cgroup, "", empty, empty, empty, "/not/really/sys/fs/cgroup", "/", controllers); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -630,7 +630,7 @@ static int testCgroupNewForSelfHybrid(const void *args G_GNUC_UNUSED) "/not/really/sys/fs/cgroup/unified", "/", controllers); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -756,7 +756,7 @@ static int testCgroupGetPercpuStats(const void *args G_GNUC_UNUSED) ret = 0;
cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); VIR_FREE(params); return ret; } @@ -789,7 +789,7 @@ static int testCgroupGetMemoryUsage(const void *args G_GNUC_UNUSED) ret = 0;
cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -849,7 +849,7 @@ testCgroupGetMemoryStat(const void *args G_GNUC_UNUSED) ret = 0;
cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -900,7 +900,7 @@ static int testCgroupGetBlkioIoServiced(const void *args G_GNUC_UNUSED) ret = 0;
cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
@@ -973,7 +973,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; }
diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index c3784bb91d..4d8ec38f88 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -315,7 +315,7 @@ int virHostValidateCGroupControllers(const char *hvname, } }
- virCgroupFree(&group); + virCgroupFree(group);
return ret; }
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
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 691bec610f..78770f5d3b 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -109,6 +109,8 @@ bool virCgroupNewIgnoreError(void); void virCgroupFree(virCgroupPtr group); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(virCgroup, virCgroupFree); + bool virCgroupHasController(virCgroupPtr cgroup, int controller); int virCgroupPathOfController(virCgroupPtr group, unsigned int controller, -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt-lxc.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c index 73daf123f0..f6391214be 100644 --- a/src/libvirt-lxc.c +++ b/src/libvirt-lxc.c @@ -289,7 +289,7 @@ int virDomainLxcEnterCGroup(virDomainPtr domain, unsigned int flags) { virConnectPtr conn; - virCgroupPtr cgroup = NULL; + g_autoptr(virCgroup) cgroup = NULL; VIR_DOMAIN_DEBUG(domain, "flags=0x%x", flags); @@ -307,12 +307,9 @@ int virDomainLxcEnterCGroup(virDomainPtr domain, if (virCgroupAddProcess(cgroup, getpid()) < 0) goto error; - virCgroupFree(cgroup); - return 0; error: virDispatchError(NULL); - virCgroupFree(cgroup); return -1; } -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/lxc/lxc_cgroup.c | 18 +++++++----------- src/lxc/lxc_container.c | 37 ++++++++++++++++--------------------- src/lxc/lxc_process.c | 6 +----- 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c index b80a8911f9..3c546861f1 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -145,31 +145,27 @@ static int virLXCCgroupGetMemStat(virCgroupPtr cgroup, int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo) { - int ret = -1; - virCgroupPtr cgroup; + g_autoptr(virCgroup) cgroup = NULL; if (virCgroupNewSelf(&cgroup) < 0) return -1; if (virLXCCgroupGetMemStat(cgroup, meminfo) < 0) - goto cleanup; + return -1; if (virLXCCgroupGetMemTotal(cgroup, meminfo) < 0) - goto cleanup; + return -1; if (virLXCCgroupGetMemUsage(cgroup, meminfo) < 0) - goto cleanup; + return -1; if (virLXCCgroupGetMemSwapTotal(cgroup, meminfo) < 0) - goto cleanup; + return -1; if (virLXCCgroupGetMemSwapUsage(cgroup, meminfo) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virCgroupFree(cgroup); - return ret; + return 0; } diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 913f4de26a..2a5f8711c4 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1594,8 +1594,7 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, size_t nttyPaths, virSecurityManagerPtr securityDriver) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; g_autofree char *sec_mount_options = NULL; g_autofree char *stateDir = NULL; @@ -1607,69 +1606,65 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, /* Before pivoting we need to identify any * cgroups controllers that are mounted */ if (virCgroupNewSelf(&cgroup) < 0) - goto cleanup; + return -1; if (virFileResolveAllLinks(LXC_STATE_DIR, &stateDir) < 0) - goto cleanup; + return -1; /* Ensure the root filesystem is mounted */ if (lxcContainerPrepareRoot(vmDef, root, sec_mount_options) < 0) - goto cleanup; + return -1; /* Gives us a private root, leaving all parent OS mounts on /.oldroot */ if (lxcContainerPivotRoot(root) < 0) - goto cleanup; + return -1; /* FIXME: we should find a way to unmount these mounts for container * even user namespace is enabled. */ if (STREQ(root->src->path, "/") && (!vmDef->idmap.nuidmap) && lxcContainerUnmountForSharedRoot(stateDir, vmDef->name) < 0) - goto cleanup; + return -1; /* Mounts the core /proc, /sys, etc filesystems */ if (lxcContainerMountBasicFS(vmDef->idmap.nuidmap, !lxcNeedNetworkNamespace(vmDef)) < 0) - goto cleanup; + return -1; /* Ensure entire root filesystem (except /.oldroot) is readonly */ if (root->readonly && lxcContainerSetReadOnly() < 0) - goto cleanup; + return -1; /* Mounts /proc/meminfo etc sysinfo */ if (lxcContainerMountProcFuse(vmDef, stateDir) < 0) - goto cleanup; + return -1; /* Now we can re-mount the cgroups controllers in the * same configuration as before */ if (virCgroupBindMount(cgroup, "/.oldroot/", sec_mount_options) < 0) - goto cleanup; + return -1; /* Mounts /dev */ if (lxcContainerMountFSDev(vmDef, stateDir) < 0) - goto cleanup; + return -1; /* Mounts /dev/pts */ if (lxcContainerMountFSDevPTS(vmDef, stateDir) < 0) - goto cleanup; + return -1; /* Setup device nodes in /dev/ */ if (lxcContainerSetupDevices(ttyPaths, nttyPaths) < 0) - goto cleanup; + return -1; /* Sets up any non-root mounts from guest config */ if (lxcContainerMountAllFS(vmDef, sec_mount_options) < 0) - goto cleanup; + return -1; /* Gets rid of all remaining mounts from host OS, including /.oldroot itself */ if (lxcContainerUnmountSubtree("/.oldroot", true) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virCgroupFree(cgroup); - return ret; + return 0; } static int lxcContainerResolveAllSymlinks(virDomainDefPtr vmDef) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index a98a090893..e392d98f5d 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -1194,7 +1194,7 @@ int virLXCProcessStart(virConnectPtr conn, virCapsPtr caps = NULL; virErrorPtr err = NULL; virLXCDriverConfigPtr cfg = virLXCDriverGetConfig(driver); - virCgroupPtr selfcgroup; + g_autoptr(virCgroup) selfcgroup = NULL; int status; g_autofree char *pidfile = NULL; @@ -1203,26 +1203,22 @@ int virLXCProcessStart(virConnectPtr conn, if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_CPUACCT)) { - 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); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'devices' cgroups controller mount")); return -1; } if (!virCgroupHasController(selfcgroup, VIR_CGROUP_CONTROLLER_MEMORY)) { - virCgroupFree(selfcgroup); virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Unable to find 'memory' cgroups controller mount")); return -1; } - virCgroupFree(selfcgroup); if (vm->def->nconsoles == 0) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 28 +++------ src/qemu/qemu_driver.c | 139 ++++++++++++++--------------------------- 2 files changed, 57 insertions(+), 110 deletions(-) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index 473ca8a414..210cac9866 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -966,26 +966,22 @@ qemuRestoreCgroupThread(virCgroupPtr cgroup, virCgroupThreadName thread, int id) { - virCgroupPtr cgroup_temp = NULL; + g_autoptr(virCgroup) cgroup_temp = NULL; g_autofree char *nodeset = NULL; - int ret = -1; if (virCgroupNewThread(cgroup, thread, id, false, &cgroup_temp) < 0) - goto cleanup; + return -1; if (virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0) - goto cleanup; + return -1; if (virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0) - goto cleanup; + return -1; if (virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virCgroupFree(cgroup_temp); - return ret; + return 0; } static void @@ -1129,8 +1125,7 @@ qemuSetupCgroupForExtDevices(virDomainObjPtr vm, virQEMUDriverPtr driver) { qemuDomainObjPrivatePtr priv = vm->privateData; - virCgroupPtr cgroup_temp = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup_temp = NULL; if (!qemuExtDevicesHasDevice(vm->def) || priv->cgroup == NULL) @@ -1147,14 +1142,9 @@ qemuSetupCgroupForExtDevices(virDomainObjPtr vm, if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, false, &cgroup_temp) < 0) - goto cleanup; + return -1; - ret = qemuExtDevicesSetupCgroup(driver, vm, cgroup_temp); - - cleanup: - virCgroupFree(cgroup_temp); - - return ret; + return qemuExtDevicesSetupCgroup(driver, vm, cgroup_temp); } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 60e043115f..825bdd9119 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -4519,7 +4519,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, virBitmapPtr tmpmap = NULL; virDomainVcpuDefPtr vcpuinfo; qemuDomainObjPrivatePtr priv = vm->privateData; - virCgroupPtr cgroup_vcpu = NULL; + g_autoptr(virCgroup) cgroup_vcpu = NULL; g_autofree char *str = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; @@ -4583,7 +4583,6 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, cleanup: virBitmapFree(tmpmap); - virCgroupFree(cgroup_vcpu); virObjectEventStateQueue(driver->domainEventState, event); return ret; } @@ -4720,7 +4719,7 @@ qemuDomainPinEmulator(virDomainPtr dom, { virQEMUDriverPtr driver = dom->conn->privateData; virDomainObjPtr vm; - virCgroupPtr cgroup_emulator = NULL; + g_autoptr(virCgroup) cgroup_emulator = NULL; virDomainDefPtr def; virDomainDefPtr persistentDef; int ret = -1; @@ -4808,8 +4807,6 @@ qemuDomainPinEmulator(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (cgroup_emulator) - virCgroupFree(cgroup_emulator); virObjectEventStateQueue(driver->domainEventState, event); virBitmapFree(pcpumap); virDomainObjEndAPI(&vm); @@ -5174,7 +5171,7 @@ qemuDomainPinIOThread(virDomainPtr dom, virDomainDefPtr persistentDef; virBitmapPtr pcpumap = NULL; qemuDomainObjPrivatePtr priv; - virCgroupPtr cgroup_iothread = NULL; + g_autoptr(virCgroup) cgroup_iothread = NULL; virObjectEventPtr event = NULL; char paramField[VIR_TYPED_PARAM_FIELD_LENGTH] = ""; g_autofree char *str = NULL; @@ -5286,8 +5283,6 @@ qemuDomainPinIOThread(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (cgroup_iothread) - virCgroupFree(cgroup_iothread); virObjectEventStateQueue(driver->domainEventState, event); virBitmapFree(pcpumap); virDomainObjEndAPI(&vm); @@ -8691,65 +8686,60 @@ static int qemuDomainSetNumaParamsLive(virDomainObjPtr vm, virBitmapPtr nodeset) { - virCgroupPtr cgroup_temp = NULL; + g_autoptr(virCgroup) cgroup_thread = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; g_autofree char *nodeset_str = NULL; virDomainNumatuneMemMode mode; size_t i = 0; - int ret = -1; if (virDomainNumatuneGetMode(vm->def->numa, -1, &mode) == 0 && mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) { virReportError(VIR_ERR_OPERATION_INVALID, "%s", _("change of nodeset for running domain " "requires strict numa mode")); - goto cleanup; + return -1; } if (!virNumaNodesetIsAvailable(nodeset)) - goto cleanup; + return -1; /* Ensure the cpuset string is formatted before passing to cgroup */ if (!(nodeset_str = virBitmapFormat(nodeset))) - goto cleanup; + return -1; if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, - false, &cgroup_temp) < 0 || - virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) - goto cleanup; - virCgroupFree(cgroup_temp); + false, &cgroup_thread) < 0 || + virCgroupSetCpusetMems(cgroup_thread, nodeset_str) < 0) + return -1; for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { + g_autoptr(virCgroup) cgroup_vcpu = NULL; virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); if (!vcpu->online) continue; if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, i, - false, &cgroup_temp) < 0 || - virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) - goto cleanup; - virCgroupFree(cgroup_temp); + false, &cgroup_vcpu) < 0 || + virCgroupSetCpusetMems(cgroup_vcpu, nodeset_str) < 0) + return -1; } for (i = 0; i < vm->def->niothreadids; i++) { + g_autoptr(virCgroup) cgroup_iothread = NULL; + if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, vm->def->iothreadids[i]->iothread_id, - false, &cgroup_temp) < 0 || - virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) - goto cleanup; - virCgroupFree(cgroup_temp); + false, &cgroup_iothread) < 0 || + virCgroupSetCpusetMems(cgroup_iothread, nodeset_str) < 0) + return -1; } /* set nodeset for root cgroup */ if (virCgroupSetCpusetMems(priv->cgroup, nodeset_str) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - virCgroupFree(cgroup_temp); - - return ret; + return 0; } static int @@ -9143,7 +9133,6 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, unsigned long long period, long long quota) { size_t i; - virCgroupPtr cgroup_vcpu = NULL; if (period == 0 && quota == 0) return 0; @@ -9152,26 +9141,21 @@ qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, return 0; for (i = 0; i < virDomainDefGetVcpusMax(vm->def); i++) { + g_autoptr(virCgroup) cgroup_vcpu = NULL; virDomainVcpuDefPtr vcpu = virDomainDefGetVcpu(vm->def, i); if (!vcpu->online) - continue; + return -1; if (virCgroupNewThread(cgroup, VIR_CGROUP_THREAD_VCPU, i, false, &cgroup_vcpu) < 0) - goto cleanup; + return -1; if (qemuSetupCgroupVcpuBW(cgroup_vcpu, period, quota) < 0) - goto cleanup; - - virCgroupFree(cgroup_vcpu); + return -1; } return 0; - - cleanup: - virCgroupFree(cgroup_vcpu); - return -1; } static int @@ -9179,24 +9163,19 @@ qemuSetEmulatorBandwidthLive(virCgroupPtr cgroup, unsigned long long period, long long quota) { - virCgroupPtr cgroup_emulator = NULL; + g_autoptr(virCgroup) cgroup_emulator = NULL; if (period == 0 && quota == 0) return 0; if (virCgroupNewThread(cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, false, &cgroup_emulator) < 0) - goto cleanup; + return -1; if (qemuSetupCgroupVcpuBW(cgroup_emulator, period, quota) < 0) - goto cleanup; + return -1; - virCgroupFree(cgroup_emulator); return 0; - - cleanup: - virCgroupFree(cgroup_emulator); - return -1; } @@ -9205,7 +9184,6 @@ qemuSetIOThreadsBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, unsigned long long period, long long quota) { size_t i; - virCgroupPtr cgroup_iothread = NULL; if (period == 0 && quota == 0) return 0; @@ -9214,22 +9192,18 @@ qemuSetIOThreadsBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, return 0; for (i = 0; i < vm->def->niothreadids; i++) { + g_autoptr(virCgroup) cgroup_iothread = NULL; + if (virCgroupNewThread(cgroup, VIR_CGROUP_THREAD_IOTHREAD, vm->def->iothreadids[i]->iothread_id, false, &cgroup_iothread) < 0) - goto cleanup; + return -1; if (qemuSetupCgroupVcpuBW(cgroup_iothread, period, quota) < 0) - goto cleanup; - - virCgroupFree(cgroup_iothread); + return -1; } return 0; - - cleanup: - virCgroupFree(cgroup_iothread); - return -1; } @@ -9571,38 +9545,32 @@ static int qemuGetVcpusBWLive(virDomainObjPtr vm, unsigned long long *period, long long *quota) { - virCgroupPtr cgroup_vcpu = NULL; + g_autoptr(virCgroup) cgroup_vcpu = NULL; qemuDomainObjPrivatePtr priv = NULL; int rc; - int ret = -1; priv = vm->privateData; if (!qemuDomainHasVcpuPids(vm)) { /* We do not create sub dir for each vcpu */ rc = qemuGetVcpuBWLive(priv->cgroup, period, quota); if (rc < 0) - goto cleanup; + return -1; if (*quota > 0) *quota /= virDomainDefGetVcpus(vm->def); - goto out; + return 0; } /* get period and quota for vcpu0 */ if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_VCPU, 0, false, &cgroup_vcpu) < 0) - goto cleanup; + return -1; rc = qemuGetVcpuBWLive(cgroup_vcpu, period, quota); if (rc < 0) - goto cleanup; + return -1; - out: - ret = 0; - - cleanup: - virCgroupFree(cgroup_vcpu); - return ret; + return 0; } static int @@ -9610,58 +9578,47 @@ qemuGetEmulatorBandwidthLive(virCgroupPtr cgroup, unsigned long long *period, long long *quota) { - virCgroupPtr cgroup_emulator = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup_emulator = NULL; /* get period and quota for emulator */ if (virCgroupNewThread(cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, false, &cgroup_emulator) < 0) - goto cleanup; + return -1; if (qemuGetVcpuBWLive(cgroup_emulator, period, quota) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virCgroupFree(cgroup_emulator); - return ret; + return 0; } static int qemuGetIOThreadsBWLive(virDomainObjPtr vm, unsigned long long *period, long long *quota) { - virCgroupPtr cgroup_iothread = NULL; + g_autoptr(virCgroup) cgroup_iothread = NULL; qemuDomainObjPrivatePtr priv = NULL; int rc; - int ret = -1; priv = vm->privateData; if (!vm->def->niothreadids) { /* We do not create sub dir for each iothread */ if ((rc = qemuGetVcpuBWLive(priv->cgroup, period, quota)) < 0) - goto cleanup; + return -1; - goto out; + return 0; } /* get period and quota for the "first" IOThread */ if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_IOTHREAD, vm->def->iothreadids[0]->iothread_id, false, &cgroup_iothread) < 0) - goto cleanup; + return -1; rc = qemuGetVcpuBWLive(cgroup_iothread, period, quota); if (rc < 0) - goto cleanup; + return -1; - out: - ret = 0; - - cleanup: - virCgroupFree(cgroup_iothread); - return ret; + return 0; } -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/util/vircgroup.c | 179 ++++++++++++++++++----------------------- src/util/vircgroupv1.c | 9 +-- 2 files changed, 81 insertions(+), 107 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 04876c6596..d408e3366f 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -662,28 +662,26 @@ virCgroupNew(pid_t pid, int controllers, virCgroupPtr *group) { + g_autoptr(virCgroup) newGroup = NULL; + VIR_DEBUG("pid=%lld path=%s parent=%p controllers=%d group=%p", (long long) pid, path, parent, controllers, group); - *group = g_new0(virCgroup, 1); + *group = NULL; + newGroup = g_new0(virCgroup, 1); if (path[0] == '/' || !parent) { - (*group)->path = g_strdup(path); + newGroup->path = g_strdup(path); } else { - (*group)->path = g_strdup_printf("%s%s%s", parent->path, + newGroup->path = g_strdup_printf("%s%s%s", parent->path, STREQ(parent->path, "") ? "" : "/", path); } - if (virCgroupDetect(*group, pid, controllers, path, parent) < 0) - goto error; + if (virCgroupDetect(newGroup, pid, controllers, path, parent) < 0) + return -1; + *group = g_steal_pointer(&newGroup); return 0; - - error: - virCgroupFree(*group); - *group = NULL; - - return -1; } @@ -821,13 +819,16 @@ virCgroupNewPartition(const char *path, int controllers, virCgroupPtr *group) { - int ret = -1; g_autofree char *parentPath = NULL; g_autofree char *newPath = NULL; - virCgroupPtr parent = NULL; + g_autoptr(virCgroup) parent = NULL; + g_autoptr(virCgroup) newGroup = NULL; + VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); + *group = NULL; + if (path[0] != '/') { virReportError(VIR_ERR_INTERNAL_ERROR, _("Partition path '%s' must start with '/'"), @@ -836,7 +837,7 @@ virCgroupNewPartition(const char *path, } if (virCgroupSetPartitionSuffix(path, &newPath) < 0) - goto cleanup; + return -1; if (STRNEQ(newPath, "/")) { char *tmp; @@ -847,25 +848,19 @@ virCgroupNewPartition(const char *path, *tmp = '\0'; if (virCgroupNew(-1, parentPath, NULL, controllers, &parent) < 0) - goto cleanup; + return -1; } - if (virCgroupNew(-1, newPath, parent, controllers, group) < 0) - goto cleanup; + if (virCgroupNew(-1, newPath, parent, controllers, &newGroup) < 0) + return -1; if (parent) { - if (virCgroupMakeGroup(parent, *group, create, VIR_CGROUP_NONE) < 0) - goto cleanup; + if (virCgroupMakeGroup(parent, newGroup, create, VIR_CGROUP_NONE) < 0) + return -1; } - ret = 0; - cleanup: - if (ret != 0) { - virCgroupFree(*group); - *group = NULL; - } - virCgroupFree(parent); - return ret; + *group = g_steal_pointer(&newGroup); + return 0; } @@ -904,13 +899,14 @@ virCgroupNewDomainPartition(virCgroupPtr partition, virCgroupPtr *group) { g_autofree char *grpname = NULL; + g_autoptr(virCgroup) newGroup = NULL; grpname = g_strdup_printf("%s.libvirt-%s", name, driver); if (virCgroupPartitionEscape(&grpname) < 0) return -1; - if (virCgroupNew(-1, grpname, partition, -1, group) < 0) + if (virCgroupNew(-1, grpname, partition, -1, &newGroup) < 0) return -1; /* @@ -923,13 +919,12 @@ virCgroupNewDomainPartition(virCgroupPtr partition, * a group for driver, is to avoid overhead to track * cumulative usage that we don't need. */ - if (virCgroupMakeGroup(partition, *group, create, + if (virCgroupMakeGroup(partition, newGroup, create, VIR_CGROUP_MEM_HIERACHY) < 0) { - virCgroupFree(*group); - *group = NULL; return -1; } + *group = g_steal_pointer(&newGroup); return 0; } @@ -953,8 +948,11 @@ virCgroupNewThread(virCgroupPtr domain, virCgroupPtr *group) { g_autofree char *name = NULL; + g_autoptr(virCgroup) newGroup = NULL; int controllers; + *group = NULL; + switch (nameval) { case VIR_CGROUP_THREAD_VCPU: name = g_strdup_printf("vcpu%d", id); @@ -975,15 +973,13 @@ virCgroupNewThread(virCgroupPtr domain, (1 << VIR_CGROUP_CONTROLLER_CPUACCT) | (1 << VIR_CGROUP_CONTROLLER_CPUSET)); - if (virCgroupNew(-1, name, domain, controllers, group) < 0) + if (virCgroupNew(-1, name, domain, controllers, &newGroup) < 0) return -1; - if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_THREAD) < 0) { - virCgroupFree(*group); - *group = NULL; + if (virCgroupMakeGroup(domain, newGroup, create, VIR_CGROUP_THREAD) < 0) return -1; - } + *group = g_steal_pointer(&newGroup); return 0; } @@ -1009,26 +1005,28 @@ virCgroupNewDetectMachine(const char *name, virCgroupPtr *group) { size_t i; + g_autoptr(virCgroup) newGroup = NULL; - if (virCgroupNewDetect(pid, controllers, group) < 0) { + *group = NULL; + + if (virCgroupNewDetect(pid, controllers, &newGroup) < 0) { if (virCgroupNewIgnoreError()) return 0; return -1; } for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) { - if ((*group)->backends[i] && - !(*group)->backends[i]->validateMachineGroup(*group, name, + if (newGroup->backends[i] && + !newGroup->backends[i]->validateMachineGroup(newGroup, name, drivername, machinename)) { VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'", name, drivername); - virCgroupFree(*group); - *group = NULL; return 0; } } + *group = g_steal_pointer(&newGroup); return 0; } @@ -1039,19 +1037,18 @@ virCgroupEnableMissingControllers(char *path, int controllers, virCgroupPtr *group) { - virCgroupPtr parent = NULL; + g_autoptr(virCgroup) parent = NULL; char *offset = path; - int ret = -1; if (virCgroupNew(pidleader, "/", NULL, controllers, &parent) < 0) - return ret; + return -1; for (;;) { - virCgroupPtr tmp; + g_autoptr(virCgroup) tmp = NULL; char *t = strchr(offset + 1, '/'); if (t) *t = '\0'; @@ -1061,27 +1058,23 @@ virCgroupEnableMissingControllers(char *path, parent, controllers, &tmp) < 0) - goto cleanup; + return -1; + + if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_SYSTEMD) < 0) + return -1; - if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_SYSTEMD) < 0) { - virCgroupFree(tmp); - goto cleanup; - } if (t) { *t = '/'; offset = t; virCgroupFree(parent); - parent = tmp; + parent = g_steal_pointer(&tmp); } else { - *group = tmp; + *group = g_steal_pointer(&tmp); break; } } - ret = 0; - cleanup: - virCgroupFree(parent); - return ret; + return 0; } @@ -1103,7 +1096,8 @@ virCgroupNewMachineSystemd(const char *name, virCgroupPtr *group) { int rv; - virCgroupPtr init; + g_autoptr(virCgroup) init = NULL; + g_autoptr(virCgroup) newGroup = NULL; g_autofree char *path = NULL; size_t i; @@ -1135,7 +1129,6 @@ virCgroupNewMachineSystemd(const char *name, break; } } - virCgroupFree(init); if (!path || STREQ(path, "/") || path[0] != '/') { VIR_DEBUG("Systemd didn't setup its controller, path=%s", @@ -1144,20 +1137,20 @@ virCgroupNewMachineSystemd(const char *name, } if (virCgroupEnableMissingControllers(path, pidleader, - controllers, group) < 0) { + controllers, &newGroup) < 0) { return -1; } - if (virCgroupAddProcess(*group, pidleader) < 0) { + if (virCgroupAddProcess(newGroup, pidleader) < 0) { virErrorPtr saved; virErrorPreserveLast(&saved); - virCgroupRemove(*group); - virCgroupFree(*group); - *group = NULL; + virCgroupRemove(newGroup); virErrorRestore(&saved); + return 0; } + *group = g_steal_pointer(&newGroup); return 0; } @@ -1179,8 +1172,8 @@ virCgroupNewMachineManual(const char *name, int controllers, virCgroupPtr *group) { - virCgroupPtr parent = NULL; - int ret = -1; + g_autoptr(virCgroup) parent = NULL; + g_autoptr(virCgroup) newGroup = NULL; VIR_DEBUG("Fallback to non-systemd setup"); if (virCgroupNewPartition(partition, @@ -1188,34 +1181,28 @@ virCgroupNewMachineManual(const char *name, controllers, &parent) < 0) { if (virCgroupNewIgnoreError()) - goto done; + return 0; - goto cleanup; + return -1; } if (virCgroupNewDomainPartition(parent, drivername, name, true, - group) < 0) - goto cleanup; + &newGroup) < 0) + return -1; - if (virCgroupAddProcess(*group, pidleader) < 0) { + if (virCgroupAddProcess(newGroup, pidleader) < 0) { virErrorPtr saved; virErrorPreserveLast(&saved); - virCgroupRemove(*group); - virCgroupFree(*group); - *group = NULL; + virCgroupRemove(newGroup); virErrorRestore(&saved); } - done: - ret = 0; - - cleanup: - virCgroupFree(parent); - return ret; + *group = g_steal_pointer(&newGroup); + return 0; } @@ -2037,22 +2024,21 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, size_t nsum, virBitmapPtr cpumap) { - int ret = -1; ssize_t i = -1; - virCgroupPtr group_vcpu = NULL; while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) { g_autofree char *buf = NULL; + g_autoptr(virCgroup) group_vcpu = NULL; char *pos; unsigned long long tmp; ssize_t j; if (virCgroupNewThread(group, VIR_CGROUP_THREAD_VCPU, i, false, &group_vcpu) < 0) - goto cleanup; + return -1; if (virCgroupGetCpuacctPercpuUsage(group_vcpu, &buf) < 0) - goto cleanup; + return -1; pos = buf; for (j = virBitmapNextSetBit(cpumap, -1); @@ -2061,18 +2047,13 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, if (virStrToLong_ull(pos, &pos, 10, &tmp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpuacct parse error")); - goto cleanup; + return -1; } sum_cpu_time[j] += tmp; } - - virCgroupFree(group_vcpu); } - ret = 0; - cleanup: - virCgroupFree(group_vcpu); - return ret; + return 0; } @@ -2519,7 +2500,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, bool killedAny = false; g_autofree char *keypath = NULL; DIR *dp = NULL; - virCgroupPtr subgroup = NULL; struct dirent *ent; int direrr; VIR_DEBUG("group=%p path=%s signum=%d pids=%p", @@ -2546,6 +2526,8 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, } while ((direrr = virDirRead(dp, &ent, keypath)) > 0) { + g_autoptr(virCgroup) subgroup = NULL; + if (ent->d_type != DT_DIR) continue; @@ -2562,8 +2544,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if (dormdir) virCgroupRemove(subgroup); - - virCgroupFree(subgroup); } if (direrr < 0) goto cleanup; @@ -2572,7 +2552,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0; cleanup: - virCgroupFree(subgroup); VIR_DIR_CLOSE(dp); return ret; } @@ -2767,15 +2746,12 @@ virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) bool virCgroupControllerAvailable(int controller) { - virCgroupPtr cgroup; - bool ret = false; + g_autoptr(virCgroup) cgroup = NULL; if (virCgroupNewSelf(&cgroup) < 0) - return ret; + return false; - ret = virCgroupHasController(cgroup, controller); - virCgroupFree(cgroup); - return ret; + return virCgroupHasController(cgroup, controller); } #else /* !__linux__ */ @@ -3567,7 +3543,7 @@ virCgroupDelThread(virCgroupPtr cgroup, virCgroupThreadName nameval, int idx) { - virCgroupPtr new_cgroup = NULL; + g_autoptr(virCgroup) new_cgroup = NULL; if (cgroup) { if (virCgroupNewThread(cgroup, nameval, idx, false, &new_cgroup) < 0) @@ -3575,7 +3551,6 @@ virCgroupDelThread(virCgroupPtr cgroup, /* Remove the offlined cgroup */ virCgroupRemove(new_cgroup); - virCgroupFree(new_cgroup); } return 0; diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 52d2a17d39..a42b7750f9 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -1470,21 +1470,20 @@ static virOnceControl virCgroupV1MemoryOnce = VIR_ONCE_CONTROL_INITIALIZER; static void virCgroupV1MemoryOnceInit(void) { - virCgroupPtr group; + g_autoptr(virCgroup) group = NULL; unsigned long long int mem_unlimited = 0ULL; if (virCgroupNew(-1, "/", NULL, -1, &group) < 0) - goto cleanup; + return; if (!virCgroupV1HasController(group, VIR_CGROUP_CONTROLLER_MEMORY)) - goto cleanup; + return; ignore_value(virCgroupGetValueU64(group, VIR_CGROUP_CONTROLLER_MEMORY, "memory.limit_in_bytes", &mem_unlimited)); - cleanup: - virCgroupFree(group); + virCgroupV1MemoryUnlimitedKB = mem_unlimited >> 10; } -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/vircgrouptest.c | 228 +++++++++++++++--------------------------- 1 file changed, 80 insertions(+), 148 deletions(-) diff --git a/tests/vircgrouptest.c b/tests/vircgrouptest.c index 632d33efcd..6c1a766fa2 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -183,7 +183,7 @@ testCgroupDetectMounts(const void *args) const struct _detectMountsData *data = args; char *parsed = NULL; const char *actual; - virCgroupPtr group = NULL; + g_autoptr(virCgroup) group = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; size_t i; @@ -217,15 +217,13 @@ testCgroupDetectMounts(const void *args) cleanup: g_unsetenv("VIR_CGROUP_MOCK_FILENAME"); VIR_FREE(parsed); - virCgroupFree(group); return result; } static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; const char *placement[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/system", [VIR_CGROUP_CONTROLLER_CPUACCT] = "/system", @@ -239,14 +237,10 @@ static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) if (virCgroupNewSelf(&cgroup) < 0) { fprintf(stderr, "Cannot create cgroup for self\n"); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "", mountsFull, links, placement, NULL, NULL, 0); - - cleanup: - virCgroupFree(cgroup); - return ret; + return validateCgroup(cgroup, "", mountsFull, links, placement, NULL, NULL, 0); } @@ -256,7 +250,7 @@ static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) virErrorPtr err = virGetLastError(); \ fprintf(stderr, "Did not get " #en " error code: %d:%d\n", \ err ? err->code : 0, err ? err->int1 : 0); \ - goto cleanup; \ + return -1; \ } } while (0) /* Asking for impossible combination since CPU is co-mounted */ @@ -264,8 +258,7 @@ static int testCgroupNewForSelf(const void *args G_GNUC_UNUSED) static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; int rv; const char *placementSmall[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/virtualmachines.partition", @@ -290,7 +283,7 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) if ((rv = virCgroupNewPartition("/virtualmachines", false, -1, &cgroup)) != -1) { fprintf(stderr, "Unexpected found /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } ENSURE_ERRNO(ENOENT); @@ -299,7 +292,7 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) (1 << VIR_CGROUP_CONTROLLER_CPU), &cgroup)) != -1) { fprintf(stderr, "Should not have created /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } ENSURE_ERRNO(EINVAL); @@ -308,7 +301,7 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) (1 << VIR_CGROUP_CONTROLLER_DEVICES), &cgroup)) != -1) { fprintf(stderr, "Should not have created /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } ENSURE_ERRNO(ENXIO); @@ -319,27 +312,22 @@ static int testCgroupNewForPartition(const void *args G_GNUC_UNUSED) (1 << VIR_CGROUP_CONTROLLER_MEMORY), &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall, NULL, NULL, 0); + rv = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall, NULL, NULL, 0); virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull, NULL, NULL, 0); - - cleanup: - virCgroupFree(cgroup); - return ret; + return validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull, NULL, NULL, 0); } static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; int rv; const char *placementFull[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/deployment.partition/production.partition", @@ -354,42 +342,37 @@ static int testCgroupNewForPartitionNested(const void *args G_GNUC_UNUSED) if ((rv = virCgroupNewPartition("/deployment/production", false, -1, &cgroup)) != -1) { fprintf(stderr, "Unexpected found /deployment/production cgroup: %d\n", -rv); - goto cleanup; + return -1; } ENSURE_ERRNO(ENOENT); /* Should not work, since we require /deployment to be pre-created */ if ((rv = virCgroupNewPartition("/deployment/production", true, -1, &cgroup)) != -1) { fprintf(stderr, "Unexpected created /deployment/production cgroup: %d\n", -rv); - goto cleanup; + return -1; } ENSURE_ERRNO(ENOENT); if ((rv = virCgroupNewPartition("/deployment", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /deployment cgroup: %d\n", -rv); - goto cleanup; + return -1; } /* Should now work */ virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/deployment/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /deployment/production cgroup: %d\n", -rv); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "/deployment.partition/production.partition", - mountsFull, links, placementFull, NULL, NULL, 0); - - cleanup: - virCgroupFree(cgroup); - return ret; + return validateCgroup(cgroup, "/deployment.partition/production.partition", + mountsFull, links, placementFull, NULL, NULL, 0); } static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; int rv; const char *placementFull[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/user/berrange.user/production.partition", @@ -404,50 +387,45 @@ static int testCgroupNewForPartitionNestedDeep(const void *args G_GNUC_UNUSED) if ((rv = virCgroupNewPartition("/user/berrange.user/production", false, -1, &cgroup)) != -1) { fprintf(stderr, "Unexpected found /user/berrange.user/production cgroup: %d\n", -rv); - goto cleanup; + return -1; } ENSURE_ERRNO(ENOENT); /* Should not work, since we require /user/berrange.user to be pre-created */ if ((rv = virCgroupNewPartition("/user/berrange.user/production", true, -1, &cgroup)) != -1) { fprintf(stderr, "Unexpected created /user/berrange.user/production cgroup: %d\n", -rv); - goto cleanup; + return -1; } ENSURE_ERRNO(ENOENT); if ((rv = virCgroupNewPartition("/user", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user cgroup: %d\n", -rv); - goto cleanup; + return -1; } virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user cgroup: %d\n", -rv); - goto cleanup; + return -1; } /* Should now work */ virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/user/berrange.user/production", true, -1, &cgroup)) != 0) { fprintf(stderr, "Failed to create /user/berrange.user/production cgroup: %d\n", -rv); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "/user/berrange.user/production.partition", - mountsFull, links, placementFull, NULL, NULL, 0); - - cleanup: - virCgroupFree(cgroup); - return ret; + return validateCgroup(cgroup, "/user/berrange.user/production.partition", + mountsFull, links, placementFull, NULL, NULL, 0); } static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) { - virCgroupPtr partitioncgroup = NULL; - virCgroupPtr domaincgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) partitioncgroup = NULL; + g_autoptr(virCgroup) domaincgroup = NULL; int rv; const char *placement[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/production.partition/foo.libvirt-lxc", @@ -462,29 +440,23 @@ static int testCgroupNewForPartitionDomain(const void *args G_GNUC_UNUSED) if ((rv = virCgroupNewPartition("/production", true, -1, &partitioncgroup)) != 0) { fprintf(stderr, "Failed to create /production cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupNewDomainPartition(partitioncgroup, "lxc", "foo", true, &domaincgroup)) != 0) { fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); - goto cleanup; + return -1; } - ret = validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); - - cleanup: - virCgroupFree(partitioncgroup); - virCgroupFree(domaincgroup); - return ret; + return validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); } static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED) { - virCgroupPtr partitioncgroup1 = NULL; - virCgroupPtr partitioncgroup2 = NULL; - virCgroupPtr partitioncgroup3 = NULL; - virCgroupPtr domaincgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) partitioncgroup1 = NULL; + g_autoptr(virCgroup) partitioncgroup2 = NULL; + g_autoptr(virCgroup) partitioncgroup3 = NULL; + g_autoptr(virCgroup) domaincgroup = NULL; int rv; const char *placement[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", @@ -499,42 +471,34 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args G_GNUC_UNUSED if ((rv = virCgroupNewPartition("/cgroup.evil", true, -1, &partitioncgroup1)) != 0) { fprintf(stderr, "Failed to create /cgroup.evil cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupNewPartition("/cgroup.evil/net_cls.evil", true, -1, &partitioncgroup2)) != 0) { fprintf(stderr, "Failed to create /cgroup.evil/cpu.evil cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupNewPartition("/cgroup.evil/net_cls.evil/_evil.evil", true, -1, &partitioncgroup3)) != 0) { fprintf(stderr, "Failed to create /cgroup.evil cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupNewDomainPartition(partitioncgroup3, "lxc", "cpu.foo", true, &domaincgroup)) != 0) { fprintf(stderr, "Cannot create LXC cgroup: %d\n", -rv); - goto cleanup; + return -1; } /* NB we're not expecting 'net_cls.evil' to be escaped, * since our fake /proc/cgroups pretends this controller * isn't compiled into the kernel */ - ret = validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); - - cleanup: - virCgroupFree(partitioncgroup3); - virCgroupFree(partitioncgroup2); - virCgroupFree(partitioncgroup1); - virCgroupFree(domaincgroup); - return ret; + return validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement, NULL, NULL, 0); } static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; const char *placement[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPU] = "/", [VIR_CGROUP_CONTROLLER_CPUACCT] = "/", @@ -547,24 +511,19 @@ static int testCgroupNewForSelfAllInOne(const void *args G_GNUC_UNUSED) if (virCgroupNewSelf(&cgroup) < 0) { fprintf(stderr, "Cannot create cgroup for self\n"); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); - - cleanup: - virCgroupFree(cgroup); - return ret; + return validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement, NULL, NULL, 0); } static int testCgroupNewForSelfLogind(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; + g_autoptr(virCgroup) cgroup = NULL; if (virCgroupNewSelf(&cgroup) >= 0) { fprintf(stderr, "Expected to fail, only systemd cgroup available.\n"); - virCgroupFree(cgroup); return -1; } @@ -574,8 +533,7 @@ static int testCgroupNewForSelfLogind(const void *args G_GNUC_UNUSED) static int testCgroupNewForSelfUnified(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; const char *empty[VIR_CGROUP_CONTROLLER_LAST] = { 0 }; unsigned int controllers = (1 << VIR_CGROUP_CONTROLLER_CPU) | @@ -586,21 +544,17 @@ static int testCgroupNewForSelfUnified(const void *args G_GNUC_UNUSED) if (virCgroupNewSelf(&cgroup) < 0) { fprintf(stderr, "Cannot create cgroup for self\n"); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "", empty, empty, empty, - "/not/really/sys/fs/cgroup", "/", controllers); - cleanup: - virCgroupFree(cgroup); - return ret; + return validateCgroup(cgroup, "", empty, empty, empty, + "/not/really/sys/fs/cgroup", "/", controllers); } static int testCgroupNewForSelfHybrid(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int ret = -1; + g_autoptr(virCgroup) cgroup = NULL; const char *empty[VIR_CGROUP_CONTROLLER_LAST] = { 0 }; const char *mounts[VIR_CGROUP_CONTROLLER_LAST] = { [VIR_CGROUP_CONTROLLER_CPUSET] = "/not/really/sys/fs/cgroup/cpuset", @@ -623,15 +577,11 @@ static int testCgroupNewForSelfHybrid(const void *args G_GNUC_UNUSED) if (virCgroupNewSelf(&cgroup) < 0) { fprintf(stderr, "Cannot create cgroup for self\n"); - goto cleanup; + return -1; } - ret = validateCgroup(cgroup, "", mounts, empty, placement, - "/not/really/sys/fs/cgroup/unified", "/", controllers); - - cleanup: - virCgroupFree(cgroup); - return ret; + return validateCgroup(cgroup, "", mounts, empty, placement, + "/not/really/sys/fs/cgroup/unified", "/", controllers); } @@ -679,7 +629,7 @@ static int testCgroupControllerAvailable(const void *args G_GNUC_UNUSED) static int testCgroupGetPercpuStats(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; + g_autoptr(virCgroup) cgroup = NULL; size_t i; int rv, ret = -1; virTypedParameterPtr params = NULL; @@ -756,50 +706,44 @@ static int testCgroupGetPercpuStats(const void *args G_GNUC_UNUSED) ret = 0; cleanup: - virCgroupFree(cgroup); VIR_FREE(params); return ret; } static int testCgroupGetMemoryUsage(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; - int rv, ret = -1; + g_autoptr(virCgroup) cgroup = NULL; + int rv; unsigned long kb; if ((rv = virCgroupNewPartition("/virtualmachines", true, (1 << VIR_CGROUP_CONTROLLER_MEMORY), &cgroup)) < 0) { fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupGetMemoryUsage(cgroup, &kb)) < 0) { fprintf(stderr, "Could not retrieve GetMemoryUsage for /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } if (kb != 1421212UL) { fprintf(stderr, "Wrong value from virCgroupGetMemoryUsage (expected %ld)\n", 1421212UL); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - virCgroupFree(cgroup); - return ret; + return 0; } static int testCgroupGetMemoryStat(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; + g_autoptr(virCgroup) cgroup = NULL; int rv; - int ret = -1; size_t i; const unsigned long long expected_values[] = { @@ -824,7 +768,7 @@ testCgroupGetMemoryStat(const void *args G_GNUC_UNUSED) (1 << VIR_CGROUP_CONTROLLER_MEMORY), &cgroup)) < 0) { fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupGetMemoryStat(cgroup, &values[0], @@ -832,7 +776,7 @@ testCgroupGetMemoryStat(const void *args G_GNUC_UNUSED) &values[3], &values[4], &values[5])) < 0) { fprintf(stderr, "Could not retrieve GetMemoryStat for /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } for (i = 0; i < G_N_ELEMENTS(expected_values); i++) { @@ -842,23 +786,19 @@ testCgroupGetMemoryStat(const void *args G_GNUC_UNUSED) "Wrong value (%llu) for %s from virCgroupGetMemoryStat " "(expected %llu)\n", values[i], names[i], (expected_values[i] >> 10)); - goto cleanup; + return -1; } } - ret = 0; - - cleanup: - virCgroupFree(cgroup); - return ret; + return 0; } static int testCgroupGetBlkioIoServiced(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; + g_autoptr(virCgroup) cgroup = NULL; size_t i; - int rv, ret = -1; + int rv; const long long expected_values[] = { 119084214273ULL, @@ -878,14 +818,14 @@ static int testCgroupGetBlkioIoServiced(const void *args G_GNUC_UNUSED) (1 << VIR_CGROUP_CONTROLLER_BLKIO), &cgroup)) < 0) { fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupGetBlkioIoServiced(cgroup, values, &values[1], &values[2], &values[3])) < 0) { fprintf(stderr, "Could not retrieve BlkioIoServiced for /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } for (i = 0; i < G_N_ELEMENTS(expected_values); i++) { @@ -893,22 +833,18 @@ static int testCgroupGetBlkioIoServiced(const void *args G_GNUC_UNUSED) fprintf(stderr, "Wrong value for %s from virCgroupBlkioIoServiced (expected %lld)\n", names[i], expected_values[i]); - goto cleanup; + return -1; } } - ret = 0; - - cleanup: - virCgroupFree(cgroup); - return ret; + return 0; } static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) { - virCgroupPtr cgroup = NULL; + g_autoptr(virCgroup) cgroup = NULL; size_t i; - int rv, ret = -1; + int rv; const long long expected_values0[] = { 59542107136ULL, 411440480256ULL, @@ -933,7 +869,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) (1 << VIR_CGROUP_CONTROLLER_BLKIO), &cgroup)) < 0) { fprintf(stderr, "Could not create /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } if ((rv = virCgroupGetBlkioIoDeviceServiced(cgroup, @@ -941,7 +877,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) values, &values[1], &values[2], &values[3])) < 0) { fprintf(stderr, "Could not retrieve BlkioIoDeviceServiced for /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } for (i = 0; i < G_N_ELEMENTS(expected_values0); i++) { @@ -949,7 +885,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) fprintf(stderr, "Wrong value for %s from virCgroupGetBlkioIoDeviceServiced (expected %lld)\n", names[i], expected_values0[i]); - goto cleanup; + return -1; } } @@ -958,7 +894,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) values, &values[1], &values[2], &values[3])) < 0) { fprintf(stderr, "Could not retrieve BlkioIoDeviceServiced for /virtualmachines cgroup: %d\n", -rv); - goto cleanup; + return -1; } for (i = 0; i < G_N_ELEMENTS(expected_values1); i++) { @@ -966,15 +902,11 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args G_GNUC_UNUSED) fprintf(stderr, "Wrong value for %s from virCgroupGetBlkioIoDeviceServiced (expected %lld)\n", names[i], expected_values1[i]); - goto cleanup; + return -1; } } - ret = 0; - - cleanup: - virCgroupFree(cgroup); - return ret; + return 0; } # define FAKEROOTDIRTEMPLATE abs_builddir "/fakerootdir-XXXXXX" -- 2.26.2
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tools/virt-host-validate-common.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 4d8ec38f88..9779eb7b3b 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -289,7 +289,7 @@ int virHostValidateCGroupControllers(const char *hvname, int controllers, virHostValidateLevel level) { - virCgroupPtr group = NULL; + g_autoptr(virCgroup) group = NULL; int ret = 0; size_t i; @@ -315,8 +315,6 @@ int virHostValidateCGroupControllers(const char *hvname, } } - virCgroupFree(group); - return ret; } #else /* !__linux__ */ -- 2.26.2
On Thu, 8 Oct 2020 16:26:53 +0200 Pavel Hrdina <phrdina@redhat.com> wrote:
Pavel Hrdina (10): qemu_cgroup: introduce qemuRestoreCgroupThread helper util: vircgroup: use GLib alloc functions util: vircgroup: change virCgroupFree to take only virCgroupPtr util: vircgroup: introduce g_autoptr() for virCgroup libvirt-lxc: use g_autoptr for virCgroup lxc: use g_autoptr for virCgroup qemu: use g_autoptr for virCgroup util: use g_autoptr for virCgroup tests: use g_autoptr for virCgroup tools: use g_autoptr for virCgroup
src/libvirt-lxc.c | 5 +- src/lxc/lxc_cgroup.c | 20 +-- src/lxc/lxc_container.c | 37 ++--- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/lxc/lxc_process.c | 9 +- src/qemu/qemu_cgroup.c | 87 ++++++----- src/qemu/qemu_domain.c | 3 +- src/qemu/qemu_driver.c | 139 ++++++------------ src/qemu/qemu_process.c | 2 +- src/util/vircgroup.c | 198 +++++++++++-------------- src/util/vircgroup.h | 4 +- src/util/vircgroupv1.c | 9 +- tests/vircgrouptest.c | 236 +++++++++++------------------- tools/virt-host-validate-common.c | 4 +- 15 files changed, 305 insertions(+), 452 deletions(-)
Aside from the minor comment on path 3, Reviewed-by: Jonathon Jongsma <jjongsma@redhat.com>
participants (2)
-
Jonathon Jongsma -
Pavel Hrdina