[libvirt] [PATCH v1 00/40] use GNU C's cleanup attribute in src/util (batch II)
This second series of patches also modifies a few files in src/util to use VIR_AUTOFREE and VIR_AUTOPTR for automatic freeing of memory and get rid of some VIR_FREE macro invocations and *Free function calls. The argument type of virCgroupFree is changed from virCgroupPtr * to virCgroupPtr. Sukrit Bhatnagar (40): util: cgroup: modify virCgroupFree to take virCgroupPtr util: error: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: buffer: typedef and Free helper for struct _virBufferEscapePair util: buffer: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: buffer: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: buffer: use VIR_AUTOPTR for aggregate types util: hash: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: cgroup: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: cgroup: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: cgroup: use VIR_AUTOPTR for aggregate types util: mdev: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: mdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: mdev: use VIR_AUTOPTR for aggregate types util: firewall: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: firewall: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: firewall: use VIR_AUTOPTR for aggregate types util: hook: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: hook: use VIR_AUTOPTR for aggregate types util: pci: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: pci: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: pci: use VIR_AUTOPTR for aggregate types util: netdevvlan: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: usb: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: usb: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: usb: use VIR_AUTOPTR for aggregate types util: scsi: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: scsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: scsi: use VIR_AUTOPTR for aggregate types util: scsivhost: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: scsivhost: use VIR_AUTOPTR for aggregate types util: hostdev: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: hostdev: use VIR_AUTOPTR for aggregate types util: hostmem: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: iptables: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: iscsi: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: iscsi: use VIR_AUTOPTR for aggregate types util: kmod: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: kmod: use VIR_AUTOPTR for aggregate types util: lease: use VIR_AUTOFREE instead of VIR_FREE for scalar types util: lease: use VIR_AUTOPTR for aggregate types 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 | 10 +- src/qemu/qemu_cgroup.c | 16 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 34 ++- src/qemu/qemu_process.c | 2 +- src/util/virbuffer.c | 34 ++- src/util/virbuffer.h | 9 +- src/util/vircgroup.c | 718 +++++++++++++++++------------------------------ src/util/vircgroup.h | 11 +- src/util/vircgrouppriv.h | 2 +- src/util/virerror.c | 1 - src/util/virerror.h | 3 + src/util/virfirewall.c | 53 ++-- src/util/virfirewall.h | 3 + src/util/virhash.c | 1 - src/util/virhash.h | 4 + src/util/virhook.c | 20 +- src/util/virhostdev.c | 162 ++++------- src/util/virhostmem.c | 57 ++-- src/util/viriptables.c | 52 ++-- src/util/viriscsi.c | 89 ++---- src/util/virkmod.c | 38 +-- src/util/virlease.c | 82 ++---- src/util/virmdev.c | 84 ++---- src/util/virmdev.h | 4 + src/util/virnetdevvlan.c | 1 - src/util/virnetdevvlan.h | 4 + src/util/virpci.c | 323 +++++++-------------- src/util/virpci.h | 4 + src/util/virscsi.c | 64 ++--- src/util/virscsi.h | 3 + src/util/virscsivhost.c | 8 +- src/util/virscsivhost.h | 3 + src/util/virusb.c | 22 +- src/util/virusb.h | 3 + tests/vircgrouptest.c | 42 +-- 41 files changed, 739 insertions(+), 1243 deletions(-) -- 1.8.3.1
Modify virCgroupFree function signature to take a value of type virCgroupPtr instead of virCgroupPtr * as the parameter. Change the argument type in all calls to virCgroupFree function from virCgroupPtr * to virCgroupPtr. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.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 | 10 ++++----- src/qemu/qemu_cgroup.c | 16 +++++++------- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_driver.c | 34 ++++++++++++++--------------- src/qemu/qemu_process.c | 2 +- src/util/vircgroup.c | 56 ++++++++++++++++++++++++------------------------ src/util/vircgroup.h | 2 +- tests/vircgrouptest.c | 42 ++++++++++++++++++------------------ 13 files changed, 88 insertions(+), 90 deletions(-) diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c index c9f2146..12be893 100644 --- a/src/libvirt-lxc.c +++ b/src/libvirt-lxc.c @@ -309,12 +309,12 @@ int virDomainLxcEnterCGroup(virDomainPtr domain, if (virCgroupAddTask(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 8e937ec..873c843 100644 --- a/src/lxc/lxc_cgroup.c +++ b/src/lxc/lxc_cgroup.c @@ -306,7 +306,7 @@ int virLXCCgroupGetMeminfo(virLXCMeminfoPtr meminfo) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -515,7 +515,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); cgroup = NULL; goto cleanup; } diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 3a1b2d6..407214f 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1815,7 +1815,7 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, cleanup: VIR_FREE(stateDir); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); VIR_FREE(sec_mount_options); return ret; } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 4e84391..7be45f8 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -296,7 +296,7 @@ static void virLXCControllerFree(virLXCControllerPtr ctrl) VIR_FREE(ctrl->nbdpids); VIR_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 b197f9d..eb0071d 100644 --- a/src/lxc/lxc_domain.c +++ b/src/lxc/lxc_domain.c @@ -172,7 +172,7 @@ virLXCDomainObjPrivateFree(void *data) { virLXCDomainObjPrivatePtr priv = data; - virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); virLXCDomainObjFreeJob(priv); VIR_FREE(priv); } diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index 14502e1..8534051 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -220,7 +220,7 @@ static void virLXCProcessCleanup(virLXCDriverPtr driver, if (priv->cgroup) { virCgroupRemove(priv->cgroup); - virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); } /* Get machined to terminate the machine as it may not have cleaned it @@ -1203,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 43e17d7..8a00ffc 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -841,7 +841,7 @@ qemuSetupCpusetMems(virDomainObjPtr vm) ret = 0; cleanup: VIR_FREE(mem_mask); - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); return ret; } @@ -920,7 +920,7 @@ qemuInitCgroup(virDomainObjPtr vm, if (!virCgroupAvailable()) goto done; - virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); if (!vm->def->resource) { virDomainResourceDefPtr res; @@ -1008,7 +1008,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm) goto cleanup; VIR_FREE(nodeset); - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); } for (i = 0; i < vm->def->niothreadids; i++) { @@ -1021,7 +1021,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm) goto cleanup; VIR_FREE(nodeset); - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); } if (virCgroupNewThread(priv->cgroup, VIR_CGROUP_THREAD_EMULATOR, 0, @@ -1035,7 +1035,7 @@ qemuRestoreCgroupState(virDomainObjPtr vm) VIR_FREE(mem_mask); VIR_FREE(nodeset); virBitmapFree(all_nodes); - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); return; error: @@ -1057,7 +1057,7 @@ qemuConnectCgroup(virDomainObjPtr vm) if (!virCgroupAvailable()) goto done; - virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); if (virCgroupNewDetectMachine(vm->def->name, "qemu", @@ -1203,7 +1203,7 @@ qemuSetupCgroupForExtDevices(virDomainObjPtr vm, ret = qemuExtDevicesSetupCgroup(driver, vm->def, cgroup_temp); cleanup: - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); return ret; } @@ -1281,7 +1281,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 9188302..26eeb8f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1920,7 +1920,7 @@ qemuDomainObjPrivateDataClear(qemuDomainObjPrivatePtr priv) virStringListFree(priv->qemuDevices); priv->qemuDevices = NULL; - virCgroupFree(&priv->cgroup); + virCgroupFree(priv->cgroup); virPerfFree(priv->perf); priv->perf = NULL; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 8cdb04e..606fd72 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5075,7 +5075,7 @@ qemuDomainPinVcpuLive(virDomainObjPtr vm, cleanup: virBitmapFree(tmpmap); - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); VIR_FREE(str); virObjectEventStateQueue(driver->domainEventState, event); return ret; @@ -5312,8 +5312,7 @@ qemuDomainPinEmulator(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (cgroup_emulator) - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); virObjectEventStateQueue(driver->domainEventState, event); VIR_FREE(str); virBitmapFree(pcpumap); @@ -5794,8 +5793,7 @@ qemuDomainPinIOThread(virDomainPtr dom, qemuDomainObjEndJob(driver, vm); cleanup: - if (cgroup_iothread) - virCgroupFree(&cgroup_iothread); + virCgroupFree(cgroup_iothread); virObjectEventStateQueue(driver->domainEventState, event); VIR_FREE(str); virBitmapFree(pcpumap); @@ -9855,7 +9853,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); @@ -9867,7 +9865,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++) { @@ -9876,13 +9874,13 @@ qemuDomainSetNumaParamsLive(virDomainObjPtr vm, false, &cgroup_temp) < 0 || virCgroupSetCpusetMems(cgroup_temp, nodeset_str) < 0) goto cleanup; - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); } ret = 0; cleanup: VIR_FREE(nodeset_str); - virCgroupFree(&cgroup_temp); + virCgroupFree(cgroup_temp); return ret; } @@ -10304,13 +10302,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; } @@ -10331,11 +10329,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; } @@ -10362,13 +10360,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; } @@ -10754,7 +10752,7 @@ qemuGetVcpusBWLive(virDomainObjPtr vm, ret = 0; cleanup: - virCgroupFree(&cgroup_vcpu); + virCgroupFree(cgroup_vcpu); return ret; } @@ -10777,7 +10775,7 @@ qemuGetEmulatorBandwidthLive(virCgroupPtr cgroup, ret = 0; cleanup: - virCgroupFree(&cgroup_emulator); + virCgroupFree(cgroup_emulator); return ret; } @@ -10813,7 +10811,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 a4b1f97..b3abaae 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2532,7 +2532,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 e810a3d..140b016 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1178,7 +1178,7 @@ virCgroupNew(pid_t pid, return 0; error: - virCgroupFree(group); + virCgroupFree(*group); *group = NULL; return -1; @@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, ret = 0; cleanup: if (ret != 0) - virCgroupFree(group); - virCgroupFree(&parent); + virCgroupFree(*group); + virCgroupFree(parent); VIR_FREE(parentPath); VIR_FREE(newPath); return ret; @@ -1447,7 +1447,7 @@ virCgroupNewDomainPartition(virCgroupPtr partition, if (virCgroupMakeGroup(partition, *group, create, VIR_CGROUP_MEM_HIERACHY) < 0) { virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); goto cleanup; } @@ -1509,7 +1509,7 @@ virCgroupNewThread(virCgroupPtr domain, if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) { virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); goto cleanup; } @@ -1550,7 +1550,7 @@ virCgroupNewDetectMachine(const char *name, true, machinename)) { VIR_DEBUG("Failed to validate machine name for '%s' driver '%s'", name, drivername); - virCgroupFree(group); + virCgroupFree(*group); return 0; } @@ -1603,7 +1603,7 @@ virCgroupNewMachineSystemd(const char *name, path = init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement; init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement = NULL; - virCgroupFree(&init); + virCgroupFree(init); if (!path || STREQ(path, "/") || path[0] != '/') { VIR_DEBUG("Systemd didn't setup its controller"); @@ -1635,13 +1635,13 @@ virCgroupNewMachineSystemd(const char *name, goto cleanup; if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_NONE) < 0) { - virCgroupFree(&tmp); + virCgroupFree(tmp); goto cleanup; } if (t) { *t = '/'; offset = t; - virCgroupFree(&parent); + virCgroupFree(parent); parent = tmp; } else { *group = tmp; @@ -1652,7 +1652,7 @@ virCgroupNewMachineSystemd(const char *name, if (virCgroupAddTask(*group, pidleader) < 0) { virErrorPtr saved = virSaveLastError(); virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); if (saved) { virSetError(saved); virFreeError(saved); @@ -1661,7 +1661,7 @@ virCgroupNewMachineSystemd(const char *name, ret = 0; cleanup: - virCgroupFree(&parent); + virCgroupFree(parent); VIR_FREE(path); return ret; } @@ -1708,7 +1708,7 @@ virCgroupNewMachineManual(const char *name, if (virCgroupAddTask(*group, pidleader) < 0) { virErrorPtr saved = virSaveLastError(); virCgroupRemove(*group); - virCgroupFree(group); + virCgroupFree(*group); if (saved) { virSetError(saved); virFreeError(saved); @@ -1719,7 +1719,7 @@ virCgroupNewMachineManual(const char *name, ret = 0; cleanup: - virCgroupFree(&parent); + virCgroupFree(parent); return ret; } @@ -1786,21 +1786,21 @@ virCgroupNewIgnoreError(void) * @group: The group structure to free */ void -virCgroupFree(virCgroupPtr *group) +virCgroupFree(virCgroupPtr group) { size_t i; - if (*group == NULL) + if (!group) return; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - VIR_FREE((*group)->controllers[i].mountPoint); - VIR_FREE((*group)->controllers[i].linkPoint); - VIR_FREE((*group)->controllers[i].placement); + VIR_FREE(group->controllers[i].mountPoint); + VIR_FREE(group->controllers[i].linkPoint); + VIR_FREE(group->controllers[i].placement); } - VIR_FREE((*group)->path); - VIR_FREE(*group); + VIR_FREE(group->path); + VIR_FREE(group); } @@ -2514,7 +2514,7 @@ virCgroupMemoryOnceInit(void) "memory.limit_in_bytes", &mem_unlimited)); cleanup: - virCgroupFree(&group); + virCgroupFree(group); virCgroupMemoryUnlimitedKB = mem_unlimited >> 10; } @@ -3158,13 +3158,13 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, sum_cpu_time[j] += tmp; } - virCgroupFree(&group_vcpu); + virCgroupFree(group_vcpu); VIR_FREE(buf); } ret = 0; cleanup: - virCgroupFree(&group_vcpu); + virCgroupFree(group_vcpu); VIR_FREE(buf); return ret; } @@ -3722,7 +3722,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if (dormdir) virCgroupRemove(subgroup); - virCgroupFree(&subgroup); + virCgroupFree(subgroup); } if (direrr < 0) goto cleanup; @@ -3731,7 +3731,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0; cleanup: - virCgroupFree(&subgroup); + virCgroupFree(subgroup); VIR_FREE(keypath); VIR_DIR_CLOSE(dp); return ret; @@ -4118,7 +4118,7 @@ virCgroupControllerAvailable(int controller) return ret; ret = virCgroupHasController(cgroup, controller); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -4250,7 +4250,7 @@ virCgroupNewIgnoreError(void) void -virCgroupFree(virCgroupPtr *group ATTRIBUTE_UNUSED) +virCgroupFree(virCgroupPtr group ATTRIBUTE_UNUSED) { virReportSystemError(ENXIO, "%s", _("Control groups not supported on this platform")); @@ -4915,7 +4915,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 d833927..e4ffd57 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -122,7 +122,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/tests/vircgrouptest.c b/tests/vircgrouptest.c index be50f3e..e5190e3 100644 --- a/tests/vircgrouptest.c +++ b/tests/vircgrouptest.c @@ -198,7 +198,7 @@ testCgroupDetectMounts(const void *args) cleanup: VIR_FREE(mounts); VIR_FREE(parsed); - virCgroupFree(&group); + virCgroupFree(group); virBufferFreeAndReset(&buf); return result; } @@ -227,7 +227,7 @@ static int testCgroupNewForSelf(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "", mountsFull, links, placement); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -304,7 +304,7 @@ static int testCgroupNewForPartition(const void *args ATTRIBUTE_UNUSED) goto cleanup; } ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsSmall, links, placementSmall); - virCgroupFree(&cgroup); + virCgroupFree(cgroup); if ((rv = virCgroupNewPartition("/virtualmachines", true, -1, &cgroup)) != 0) { fprintf(stderr, "Cannot create /virtualmachines cgroup: %d\n", -rv); @@ -313,7 +313,7 @@ static int testCgroupNewForPartition(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "/virtualmachines.partition", mountsFull, links, placementFull); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -353,7 +353,7 @@ static int testCgroupNewForPartitionNested(const void *args ATTRIBUTE_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; @@ -363,7 +363,7 @@ static int testCgroupNewForPartitionNested(const void *args ATTRIBUTE_UNUSED) mountsFull, links, placementFull); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -402,14 +402,14 @@ static int testCgroupNewForPartitionNestedDeep(const void *args ATTRIBUTE_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; @@ -419,7 +419,7 @@ static int testCgroupNewForPartitionNestedDeep(const void *args ATTRIBUTE_UNUSED mountsFull, links, placementFull); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -455,8 +455,8 @@ static int testCgroupNewForPartitionDomain(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(domaincgroup, "/production.partition/foo.libvirt-lxc", mountsFull, links, placement); cleanup: - virCgroupFree(&partitioncgroup); - virCgroupFree(&domaincgroup); + virCgroupFree(partitioncgroup); + virCgroupFree(domaincgroup); return ret; } @@ -506,10 +506,10 @@ static int testCgroupNewForPartitionDomainEscaped(const void *args ATTRIBUTE_UNU ret = validateCgroup(domaincgroup, "/_cgroup.evil/net_cls.evil/__evil.evil/_cpu.foo.libvirt-lxc", mountsFull, links, placement); cleanup: - virCgroupFree(&partitioncgroup3); - virCgroupFree(&partitioncgroup2); - virCgroupFree(&partitioncgroup1); - virCgroupFree(&domaincgroup); + virCgroupFree(partitioncgroup3); + virCgroupFree(partitioncgroup2); + virCgroupFree(partitioncgroup1); + virCgroupFree(domaincgroup); return ret; } @@ -535,7 +535,7 @@ static int testCgroupNewForSelfAllInOne(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "", mountsAllInOne, linksAllInOne, placement); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -563,7 +563,7 @@ static int testCgroupNewForSelfLogind(const void *args ATTRIBUTE_UNUSED) ret = validateCgroup(cgroup, "", mountsLogind, linksLogind, placement); cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -690,7 +690,7 @@ static int testCgroupGetPercpuStats(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); VIR_FREE(params); return ret; } @@ -723,7 +723,7 @@ static int testCgroupGetMemoryUsage(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -773,7 +773,7 @@ static int testCgroupGetBlkioIoServiced(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } @@ -846,7 +846,7 @@ static int testCgroupGetBlkioIoDeviceServiced(const void *args ATTRIBUTE_UNUSED) ret = 0; cleanup: - virCgroupFree(&cgroup); + virCgroupFree(cgroup); return ret; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
Modify virCgroupFree function signature to take a value of type virCgroupPtr instead of virCgroupPtr * as the parameter.
Change the argument type in all calls to virCgroupFree function from virCgroupPtr * to virCgroupPtr.
^This sentence doesn't add any useful information. Instead, the commit message should add information about why we're performing this change, i.e. in order to enable usage of VIR_AUTOPTR with cgroup module or something alike. Also, this patch is oddly placed, IMHO it should come before patch 8, where the other work on cgroup module is done. With that: Reviewed-by: Erik Skultety <eskultet@redhat.com> ...
@@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, ret = 0; cleanup: if (ret != 0) - virCgroupFree(group); - virCgroupFree(&parent); + virCgroupFree(*group); + virCgroupFree(parent);
Since you're already touching the code, I'd appreciate another "adjustment" patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW where we're passing a reference to a pointer in order to change the original pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do some cleanup. Feel free to let me know if none of what I just wrote is clear. Erik
On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
Modify virCgroupFree function signature to take a value of type virCgroupPtr instead of virCgroupPtr * as the parameter.
Change the argument type in all calls to virCgroupFree function from virCgroupPtr * to virCgroupPtr.
^This sentence doesn't add any useful information. Instead, the commit message should add information about why we're performing this change, i.e. in order to enable usage of VIR_AUTOPTR with cgroup module or something alike. Also, this patch is oddly placed, IMHO it should come before patch 8, where the other work on cgroup module is done.
With that: Reviewed-by: Erik Skultety <eskultet@redhat.com>
...
@@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, ret = 0; cleanup: if (ret != 0) - virCgroupFree(group); - virCgroupFree(&parent); + virCgroupFree(*group); + virCgroupFree(parent);
Since you're already touching the code, I'd appreciate another "adjustment" patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW where we're passing a reference to a pointer in order to change the original pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do some cleanup. Feel free to let me know if none of what I just wrote is clear.
I am assuming that you are referring to `group` variable. If so, then I cannot apply cleanup attribute to function parameters and `group` is one of them.
On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
Modify virCgroupFree function signature to take a value of type virCgroupPtr instead of virCgroupPtr * as the parameter.
Change the argument type in all calls to virCgroupFree function from virCgroupPtr * to virCgroupPtr.
^This sentence doesn't add any useful information. Instead, the commit message should add information about why we're performing this change, i.e. in order to enable usage of VIR_AUTOPTR with cgroup module or something alike. Also, this patch is oddly placed, IMHO it should come before patch 8, where the other work on cgroup module is done.
With that: Reviewed-by: Erik Skultety <eskultet@redhat.com>
...
@@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, ret = 0; cleanup: if (ret != 0) - virCgroupFree(group); - virCgroupFree(&parent); + virCgroupFree(*group); + virCgroupFree(parent);
Since you're already touching the code, I'd appreciate another "adjustment" patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW where we're passing a reference to a pointer in order to change the original pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do some cleanup. Feel free to let me know if none of what I just wrote is clear.
I am assuming that you are referring to `group` variable. If so, then I cannot apply cleanup attribute to function parameters and `group` is one of them.
I didn't mean using a cleanup attribute. What I meant was making the necessary adjustments in order to get rid of the 'if(ret != 0)' check, since you're already touching the code I thought why not going one step further... Erik
On Tue, 24 Jul 2018 at 11:33, Erik Skultety <eskultet@redhat.com> wrote:
On Mon, Jul 23, 2018 at 08:19:21PM +0530, Sukrit Bhatnagar wrote:
On Mon, 23 Jul 2018 at 16:29, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:33PM +0530, Sukrit Bhatnagar wrote:
Modify virCgroupFree function signature to take a value of type virCgroupPtr instead of virCgroupPtr * as the parameter.
Change the argument type in all calls to virCgroupFree function from virCgroupPtr * to virCgroupPtr.
^This sentence doesn't add any useful information. Instead, the commit message should add information about why we're performing this change, i.e. in order to enable usage of VIR_AUTOPTR with cgroup module or something alike. Also, this patch is oddly placed, IMHO it should come before patch 8, where the other work on cgroup module is done.
With that: Reviewed-by: Erik Skultety <eskultet@redhat.com>
...
@@ -1379,8 +1379,8 @@ virCgroupNewPartition(const char *path, ret = 0; cleanup: if (ret != 0) - virCgroupFree(group); - virCgroupFree(&parent); + virCgroupFree(*group); + virCgroupFree(parent);
Since you're already touching the code, I'd appreciate another "adjustment" patch where occurrences like ^this will be replaced by a VIR_STEAL_PTR, IOW where we're passing a reference to a pointer in order to change the original pointer, we should use a VIR_STEAL_PTR just before the cleanup section, so that we don't need an if (ret != 0) or if (ret < 0) check only to conditionally do some cleanup. Feel free to let me know if none of what I just wrote is clear.
I am assuming that you are referring to `group` variable. If so, then I cannot apply cleanup attribute to function parameters and `group` is one of them.
I didn't mean using a cleanup attribute. What I meant was making the necessary adjustments in order to get rid of the 'if(ret != 0)' check, since you're already touching the code I thought why not going one step further...
You mean something like this? VIR_AUTOPTR(virCgroup) ptr = NULL; ... return 0; cleanup: VIR_STEAL_PTR(ptr, *group); return -1;
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. The type virError and the declaration of virFreeError is done in include/libvirt/virterror.h which is pulled in by internal.h. When a variable of type virErrorPtr is declared using VIR_AUTOPTR, the function virFreeError will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virerror.c | 1 - src/util/virerror.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virerror.c b/src/util/virerror.c index 5f26d59..4688e01 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -30,7 +30,6 @@ #include "virerror.h" #include "datatypes.h" #include "virlog.h" -#include "viralloc.h" #include "virthread.h" #include "virutil.h" #include "virstring.h" diff --git a/src/util/virerror.h b/src/util/virerror.h index 760bfa4..31577c5 100644 --- a/src/util/virerror.h +++ b/src/util/virerror.h @@ -23,6 +23,7 @@ # define __VIRT_ERROR_H_ # include "internal.h" +# include "viralloc.h" # define VIR_ERROR_MAX_LENGTH 1024 @@ -205,4 +206,6 @@ bool virLastErrorIsSystemErrno(int errnum); void virErrorPreserveLast(virErrorPtr *saveerr); void virErrorRestore(virErrorPtr *savederr); +VIR_DEFINE_AUTOPTR_FUNC(virError, virFreeError) + #endif -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:34PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
The type virError and the declaration of virFreeError is done in include/libvirt/virterror.h which is pulled in by internal.h.
^This sentence is not needed. Reviewed-by: Erik Skultety <eskultet@redhat.com>
Create typedefs virBufferEscapePair and virBufferEscapePairPtr for struct _virBufferEscapePair for cleaner code and for use with cleanup macros. Also create a dedicated Free helper virBufferEscapePairFree. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 3d6defb..8076cd3 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -648,11 +648,23 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, } +typedef struct _virBufferEscapePair virBufferEscapePair; +typedef virBufferEscapePair *virBufferEscapePairPtr; + struct _virBufferEscapePair { char escape; char *toescape; }; +static void +virBufferEscapePairFree(virBufferEscapePairPtr pair) +{ + if (!pair) + return; + + VIR_FREE(pair); +} + /** * virBufferEscapeN: @@ -678,8 +690,8 @@ virBufferEscapeN(virBufferPtr buf, char *escaped = NULL; char *out; const char *cur; - struct _virBufferEscapePair escapeItem; - struct _virBufferEscapePair *escapeList = NULL; + virBufferEscapePair escapeItem; + virBufferEscapePairPtr escapeList = NULL; size_t nescapeList = 0; va_list ap; -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:35PM +0530, Sukrit Bhatnagar wrote:
Create typedefs virBufferEscapePair and virBufferEscapePairPtr for struct _virBufferEscapePair for cleaner code and for use with cleanup macros.
Also create a dedicated Free helper virBufferEscapePairFree.
Usually, a sentence like ^this indicates, that the change itself should come separately.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 3d6defb..8076cd3 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -648,11 +648,23 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, }
+typedef struct _virBufferEscapePair virBufferEscapePair; +typedef virBufferEscapePair *virBufferEscapePairPtr;
^This should come as a separate patch.
+ struct _virBufferEscapePair { char escape; char *toescape; };
+static void +virBufferEscapePairFree(virBufferEscapePairPtr pair) +{ + if (!pair) + return; + + VIR_FREE(pair); +}
Patch doesn't compile, please make sure that each patch can pass make check && make syntax-check. ^This function should be introduced in the following patch.
+
/** * virBufferEscapeN: @@ -678,8 +690,8 @@ virBufferEscapeN(virBufferPtr buf, char *escaped = NULL; char *out; const char *cur; - struct _virBufferEscapePair escapeItem; - struct _virBufferEscapePair *escapeList = NULL; + virBufferEscapePair escapeItem; + virBufferEscapePairPtr escapeList = NULL;
^This should come together with the typedef above. Erik
On Mon, Jul 23, 2018 at 01:19:49PM +0200, Erik Skultety wrote:
On Sat, Jul 21, 2018 at 05:36:35PM +0530, Sukrit Bhatnagar wrote:
Create typedefs virBufferEscapePair and virBufferEscapePairPtr for struct _virBufferEscapePair for cleaner code and for use with cleanup macros.
Also create a dedicated Free helper virBufferEscapePairFree.
Usually, a sentence like ^this indicates, that the change itself should come separately.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 3d6defb..8076cd3 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -648,11 +648,23 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, }
+typedef struct _virBufferEscapePair virBufferEscapePair; +typedef virBufferEscapePair *virBufferEscapePairPtr;
^This should come as a separate patch.
+ struct _virBufferEscapePair { char escape; char *toescape; };
+static void +virBufferEscapePairFree(virBufferEscapePairPtr pair) +{ + if (!pair) + return; + + VIR_FREE(pair); +}
Patch doesn't compile, please make sure that each patch can pass make check && make syntax-check.
^This function should be introduced in the following patch.
An idea to consider would be to do VIR_STRDUP on the toescape strings, so that this free helper would do VIR_FREE(toescape), that might make more sense in terms of logic. It's also safer, since we can't predict the lifetime of the string passed by the caller, they might as well free it immediately after returning from a helper, assuming the helper created their own copy of the string. Erik
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When variables of type virBufferPtr and virBufferEscapePairPtr are declared using VIR_AUTOPTR, the functions virBufferFreeAndReset and virBufferEscapePairFree, respectively, will be run automatically on them when they go out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 2 +- src/util/virbuffer.h | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 8076cd3..65f4a08 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -31,7 +31,6 @@ #define __VIR_BUFFER_C__ #include "virbuffer.h" -#include "viralloc.h" #include "virerror.h" #include "virstring.h" @@ -664,6 +663,7 @@ virBufferEscapePairFree(virBufferEscapePairPtr pair) VIR_FREE(pair); } +VIR_DEFINE_AUTOPTR_FUNC(virBufferEscapePair, virBufferEscapePairFree) /** diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index e95ee87..3b31060 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -23,10 +23,13 @@ #ifndef __VIR_BUFFER_H__ # define __VIR_BUFFER_H__ -# include "internal.h" - # include <stdarg.h> +# include "internal.h" + +# include "viralloc.h" + + /** * virBuffer: * @@ -119,4 +122,6 @@ int virBufferGetIndent(const virBuffer *buf, bool dynamic); void virBufferTrim(virBufferPtr buf, const char *trim, int len); void virBufferAddStr(virBufferPtr buf, const char *str); +VIR_DEFINE_AUTOPTR_FUNC(virBuffer, virBufferFreeAndReset) + #endif /* __VIR_BUFFER_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:36PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When variables of type virBufferPtr and virBufferEscapePairPtr are declared using VIR_AUTOPTR, the functions virBufferFreeAndReset and virBufferEscapePairFree, respectively, will be run automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 2 +- src/util/virbuffer.h | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 8076cd3..65f4a08 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -31,7 +31,6 @@ #define __VIR_BUFFER_C__
#include "virbuffer.h" -#include "viralloc.h" #include "virerror.h" #include "virstring.h"
@@ -664,6 +663,7 @@ virBufferEscapePairFree(virBufferEscapePairPtr pair)
VIR_FREE(pair); } +VIR_DEFINE_AUTOPTR_FUNC(virBufferEscapePair, virBufferEscapePairFree)
I'm wondering whether we shouldn't export virBufferEscapePair struct to other potential callers just to make things cleaner by having the VIR_DEFINE_AUTOPTR_FUNC macros coupled together, but that can be done ad-hoc once someone needs it, I'm fine with this change too.
/** diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index e95ee87..3b31060 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -23,10 +23,13 @@ #ifndef __VIR_BUFFER_H__ # define __VIR_BUFFER_H__
-# include "internal.h" - # include <stdarg.h>
+# include "internal.h" +
^This newline is redundant.
+# include "viralloc.h" + + /** * virBuffer: * @@ -119,4 +122,6 @@ int virBufferGetIndent(const virBuffer *buf, bool dynamic); void virBufferTrim(virBufferPtr buf, const char *trim, int len); void virBufferAddStr(virBufferPtr buf, const char *str);
+VIR_DEFINE_AUTOPTR_FUNC(virBuffer, virBufferFreeAndReset) + #endif /* __VIR_BUFFER_H__ */ -- 1.8.3.1
If you squash the static definition of virBufferEscapePairFree into this patch: Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 65f4a08..a35feb6 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -455,7 +455,8 @@ void virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur; const char forbidden_characters[] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, @@ -532,7 +533,6 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) *out = 0; virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); } /** @@ -611,7 +611,8 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, const char *format, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur; if ((format == NULL) || (buf == NULL) || (str == NULL)) @@ -643,7 +644,6 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, *out = 0; virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); } @@ -687,7 +687,7 @@ virBufferEscapeN(virBufferPtr buf, { int len; size_t i; - char *escaped = NULL; + VIR_AUTOFREE(char *) escaped = NULL; char *out; const char *cur; virBufferEscapePair escapeItem; @@ -750,7 +750,6 @@ virBufferEscapeN(virBufferPtr buf, cleanup: va_end(ap); VIR_FREE(escapeList); - VIR_FREE(escaped); } @@ -815,7 +814,8 @@ void virBufferEscapeShell(virBufferPtr buf, const char *str) { int len; - char *escaped, *out; + VIR_AUTOFREE(char *) escaped = NULL; + char *out; const char *cur; if ((buf == NULL) || (str == NULL)) @@ -859,7 +859,6 @@ virBufferEscapeShell(virBufferPtr buf, const char *str) *out = 0; virBufferAdd(buf, escaped, -1); - VIR_FREE(escaped); } /** -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:37PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index a35feb6..d2827f6 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -691,7 +691,7 @@ virBufferEscapeN(virBufferPtr buf, char *out; const char *cur; virBufferEscapePair escapeItem; - virBufferEscapePairPtr escapeList = NULL; + VIR_AUTOPTR(virBufferEscapePair) escapeList = NULL; size_t nescapeList = 0; va_list ap; @@ -749,7 +749,6 @@ virBufferEscapeN(virBufferPtr buf, cleanup: va_end(ap); - VIR_FREE(escapeList); } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:38PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Try considering the idea I had about the virBufferEscapePairFree function. Other than that, the patch is fine. Erik
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When variables of type virHashTablePtr are declared using VIR_AUTOPTR, the function virHashFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhash.c | 1 - src/util/virhash.h | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virhash.c b/src/util/virhash.c index ecda55d..006ffd8 100644 --- a/src/util/virhash.c +++ b/src/util/virhash.c @@ -26,7 +26,6 @@ #include "virerror.h" #include "virhash.h" -#include "viralloc.h" #include "virlog.h" #include "virhashcode.h" #include "virrandom.h" diff --git a/src/util/virhash.h b/src/util/virhash.h index 5b24fc0..dd789c6 100644 --- a/src/util/virhash.h +++ b/src/util/virhash.h @@ -16,6 +16,8 @@ # include <stdint.h> # include <stdbool.h> +# include "viralloc.h" + /* * The hash table. */ @@ -200,4 +202,6 @@ void *virHashSearch(const virHashTable *table, virHashSearcher iter, /* Convenience for when VIR_FREE(value) is sufficient as a data freer. */ void virHashValueFree(void *value, const void *name); +VIR_DEFINE_AUTOPTR_FUNC(virHashTable, virHashFree) + #endif /* ! __VIR_HASH_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:39PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When variables of type virHashTablePtr are declared using VIR_AUTOPTR, the function virHashFree will be run automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virCgroupPtr is declared using VIR_AUTOPTR, the function virCgroupFree will be run automatically on it when it goes out of scope. This commit also adds an intermediate typedef for virCgroup type for use with the cleanup macros by renaming the struct in src/util/vircgrouppriv.h from virCgroup to _virCgroup. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/vircgroup.c | 1 - src/util/vircgroup.h | 9 +++++++-- src/util/vircgrouppriv.h | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 140b016..bc5f774 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -50,7 +50,6 @@ #include "vircgrouppriv.h" #include "virutil.h" -#include "viralloc.h" #include "virerror.h" #include "virlog.h" #include "virfile.h" diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index e4ffd57..065861d 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -27,9 +27,11 @@ # include "virutil.h" # include "virbitmap.h" +# include "viralloc.h" -struct virCgroup; -typedef struct virCgroup *virCgroupPtr; +struct _virCgroup; +typedef struct _virCgroup virCgroup; +typedef virCgroup *virCgroupPtr; enum { VIR_CGROUP_CONTROLLER_CPU, @@ -297,4 +299,7 @@ int virCgroupSetOwner(virCgroupPtr cgroup, int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller); bool virCgroupControllerAvailable(int controller); + +VIR_DEFINE_AUTOPTR_FUNC(virCgroup, virCgroupFree) + #endif /* __VIR_CGROUP_H__ */ diff --git a/src/util/vircgrouppriv.h b/src/util/vircgrouppriv.h index 722863e..a72bee1 100644 --- a/src/util/vircgrouppriv.h +++ b/src/util/vircgrouppriv.h @@ -42,7 +42,7 @@ struct virCgroupController { char *placement; }; -struct virCgroup { +struct _virCgroup { char *path; struct virCgroupController controllers[VIR_CGROUP_CONTROLLER_LAST]; -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:40PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When a variable of type virCgroupPtr is declared using VIR_AUTOPTR, the function virCgroupFree will be run automatically on it when it goes out of scope.
This commit also adds an intermediate typedef for virCgroup type for use with the cleanup macros by renaming the struct in src/util/vircgrouppriv.h from virCgroup to _virCgroup.
How about: ... typedef to virCgroup type in order to make use of the VIR_DEFINE_AUTOPTR_FUNC macro. Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/vircgroup.c | 528 ++++++++++++++++++--------------------------------- 1 file changed, 181 insertions(+), 347 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index bc5f774..4bb4408 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -162,7 +162,7 @@ virCgroupPartitionNeedsEscaping(const char *path) { FILE *fp = NULL; int ret = 0; - char *line = NULL; + VIR_AUTOFREE(char *) line = NULL; size_t buflen; /* If it starts with 'cgroup.' or a '_' of any @@ -222,7 +222,6 @@ virCgroupPartitionNeedsEscaping(const char *path) } cleanup: - VIR_FREE(line); VIR_FORCE_FCLOSE(fp); return ret; } @@ -255,41 +254,40 @@ virCgroupValidateMachineGroup(virCgroupPtr group, const char *machinename) { size_t i; - bool valid = false; - char *partname = NULL; - char *scopename_old = NULL; - char *scopename_new = NULL; - char *partmachinename = NULL; + VIR_AUTOFREE(char *) partname = NULL; + VIR_AUTOFREE(char *) scopename_old = NULL; + VIR_AUTOFREE(char *) scopename_new = NULL; + VIR_AUTOFREE(char *) partmachinename = NULL; if (virAsprintf(&partname, "%s.libvirt-%s", name, drivername) < 0) - goto cleanup; + return false; if (virCgroupPartitionEscape(&partname) < 0) - goto cleanup; + return false; if (machinename && (virAsprintf(&partmachinename, "%s.libvirt-%s", machinename, drivername) < 0 || virCgroupPartitionEscape(&partmachinename) < 0)) - goto cleanup; + return false; if (!(scopename_old = virSystemdMakeScopeName(name, drivername, true))) - goto cleanup; + return false; /* We should keep trying even if this failed */ if (!machinename) virResetLastError(); else if (!(scopename_new = virSystemdMakeScopeName(machinename, drivername, false))) - goto cleanup; + return false; if (virCgroupPartitionEscape(&scopename_old) < 0) - goto cleanup; + return false; if (scopename_new && virCgroupPartitionEscape(&scopename_new) < 0) - goto cleanup; + return false; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { char *tmp; @@ -302,7 +300,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, tmp = strrchr(group->controllers[i].placement, '/'); if (!tmp) - goto cleanup; + return false; if (stripEmulatorSuffix && (i == VIR_CGROUP_CONTROLLER_CPU || @@ -312,7 +310,7 @@ virCgroupValidateMachineGroup(virCgroupPtr group, *tmp = '\0'; tmp = strrchr(group->controllers[i].placement, '/'); if (!tmp) - goto cleanup; + return false; } tmp++; @@ -328,18 +326,11 @@ virCgroupValidateMachineGroup(virCgroupPtr group, tmp, virCgroupControllerTypeToString(i), name, NULLSTR(machinename), partname, scopename_old, NULLSTR(scopename_new)); - goto cleanup; + return false; } } - valid = true; - - cleanup: - VIR_FREE(partmachinename); - VIR_FREE(partname); - VIR_FREE(scopename_old); - VIR_FREE(scopename_new); - return valid; + return true; } @@ -377,7 +368,6 @@ virCgroupDetectMountsFromFile(virCgroupPtr group, FILE *mounts = NULL; struct mntent entry; char buf[CGROUP_MAX_VAL]; - char *linksrc = NULL; int ret = -1; mounts = fopen(path, "r"); @@ -432,8 +422,9 @@ virCgroupDetectMountsFromFile(virCgroupPtr group, /* If it is a co-mount it has a filename like "cpu,cpuacct" * and we must identify the symlink path */ if (checkLinks && strchr(tmp2 + 1, ',')) { + VIR_AUTOFREE(char *) linksrc = NULL; + *tmp2 = '\0'; - VIR_FREE(linksrc); if (virAsprintf(&linksrc, "%s/%s", entry.mnt_dir, typestr) < 0) goto cleanup; @@ -467,7 +458,6 @@ virCgroupDetectMountsFromFile(virCgroupPtr group, ret = 0; cleanup: - VIR_FREE(linksrc); VIR_FORCE_FCLOSE(mounts); return ret; } @@ -546,7 +536,7 @@ virCgroupDetectPlacement(virCgroupPtr group, FILE *mapping = NULL; char line[1024]; int ret = -1; - char *procfile; + VIR_AUTOFREE(char *) procfile = NULL; VIR_DEBUG("Detecting placement for pid %lld path %s", (long long) pid, path); @@ -627,9 +617,7 @@ virCgroupDetectPlacement(virCgroupPtr group, ret = 0; cleanup: - VIR_FREE(procfile); VIR_FORCE_FCLOSE(mapping); - return ret; } @@ -785,8 +773,7 @@ virCgroupSetValueStr(virCgroupPtr group, const char *key, const char *value) { - int ret = -1; - char *keypath = NULL; + VIR_AUTOFREE(char *) keypath = NULL; char *tmp = NULL; if (virCgroupPathOfController(group, controller, key, &keypath) < 0) @@ -799,18 +786,14 @@ virCgroupSetValueStr(virCgroupPtr group, virReportSystemError(errno, _("Invalid value '%s' for '%s'"), value, tmp + 1); - goto cleanup; + return -1; } virReportSystemError(errno, _("Unable to write to '%s'"), keypath); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(keypath); - return ret; + return 0; } @@ -820,8 +803,8 @@ virCgroupGetValueStr(virCgroupPtr group, const char *key, char **value) { - char *keypath = NULL; - int ret = -1, rc; + VIR_AUTOFREE(char *) keypath = NULL; + int rc; *value = NULL; @@ -833,18 +816,14 @@ virCgroupGetValueStr(virCgroupPtr group, if ((rc = virFileReadAll(keypath, 1024*1024, value)) < 0) { virReportSystemError(errno, _("Unable to read from '%s'"), keypath); - goto cleanup; + return -1; } /* Terminated with '\n' has sometimes harmful effects to the caller */ if (rc > 0 && (*value)[rc - 1] == '\n') (*value)[rc - 1] = '\0'; - ret = 0; - - cleanup: - VIR_FREE(keypath); - return ret; + return 0; } @@ -855,8 +834,8 @@ virCgroupGetValueForBlkDev(virCgroupPtr group, const char *path, char **value) { - char *prefix = NULL; - char *str = NULL; + VIR_AUTOFREE(char *) prefix = NULL; + VIR_AUTOFREE(char *) str = NULL; char **lines = NULL; int ret = -1; @@ -874,8 +853,6 @@ virCgroupGetValueForBlkDev(virCgroupPtr group, ret = 0; error: - VIR_FREE(str); - VIR_FREE(prefix); virStringListFree(lines); return ret; } @@ -887,17 +864,12 @@ virCgroupSetValueU64(virCgroupPtr group, const char *key, unsigned long long int value) { - char *strval = NULL; - int ret; + VIR_AUTOFREE(char *) strval = NULL; if (virAsprintf(&strval, "%llu", value) < 0) return -1; - ret = virCgroupSetValueStr(group, controller, key, strval); - - VIR_FREE(strval); - - return ret; + return virCgroupSetValueStr(group, controller, key, strval); } @@ -907,17 +879,12 @@ virCgroupSetValueI64(virCgroupPtr group, const char *key, long long int value) { - char *strval = NULL; - int ret; + VIR_AUTOFREE(char *) strval = NULL; if (virAsprintf(&strval, "%lld", value) < 0) return -1; - ret = virCgroupSetValueStr(group, controller, key, strval); - - VIR_FREE(strval); - - return ret; + return virCgroupSetValueStr(group, controller, key, strval); } @@ -927,24 +894,19 @@ virCgroupGetValueI64(virCgroupPtr group, const char *key, long long int *value) { - char *strval = NULL; - int ret = -1; + VIR_AUTOFREE(char *) strval = NULL; if (virCgroupGetValueStr(group, controller, key, &strval) < 0) - goto cleanup; + return -1; if (virStrToLong_ll(strval, NULL, 10, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), strval); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(strval); - return ret; + return 0; } @@ -954,24 +916,19 @@ virCgroupGetValueU64(virCgroupPtr group, const char *key, unsigned long long int *value) { - char *strval = NULL; - int ret = -1; + VIR_AUTOFREE(char *) strval = NULL; if (virCgroupGetValueStr(group, controller, key, &strval) < 0) - goto cleanup; + return -1; if (virStrToLong_ull(strval, NULL, 10, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), strval); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(strval); - return ret; + return 0; } @@ -987,7 +944,7 @@ virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group) VIR_DEBUG("Setting up inheritance %s -> %s", parent->path, group->path); for (i = 0; i < ARRAY_CARDINALITY(inherit_values); i++) { - char *value; + VIR_AUTOFREE(char *) value = NULL; if (virCgroupGetValueStr(parent, VIR_CGROUP_CONTROLLER_CPUSET, @@ -1000,11 +957,8 @@ virCgroupCpuSetInherit(virCgroupPtr parent, virCgroupPtr group) if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_CPUSET, inherit_values[i], - value) < 0) { - VIR_FREE(value); + value) < 0) return -1; - } - VIR_FREE(value); } return 0; @@ -1043,11 +997,10 @@ virCgroupMakeGroup(virCgroupPtr parent, unsigned int flags) { size_t i; - int ret = -1; VIR_DEBUG("Make group %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; /* We must never mkdir() in systemd's hierarchy */ if (i == VIR_CGROUP_CONTROLLER_SYSTEMD) { @@ -1073,10 +1026,8 @@ virCgroupMakeGroup(virCgroupPtr parent, if (!virFileExists(path)) { if (!create || mkdir(path, 0755) < 0) { - if (errno == EEXIST) { - VIR_FREE(path); + if (errno == EEXIST) continue; - } /* With a kernel that doesn't support multi-level directory * for blkio controller, libvirt will fail and disable all * other controllers even though they are available. So @@ -1084,24 +1035,20 @@ virCgroupMakeGroup(virCgroupPtr parent, if (i == VIR_CGROUP_CONTROLLER_BLKIO) { VIR_DEBUG("Ignoring mkdir failure with blkio controller. Kernel probably too old"); VIR_FREE(group->controllers[i].mountPoint); - VIR_FREE(path); continue; } else { virReportSystemError(errno, _("Failed to create controller %s for group"), virCgroupControllerTypeToString(i)); - VIR_FREE(path); - goto cleanup; + return -1; } } if (group->controllers[VIR_CGROUP_CONTROLLER_CPUSET].mountPoint != NULL && (i == VIR_CGROUP_CONTROLLER_CPUSET || STREQ(group->controllers[i].mountPoint, group->controllers[VIR_CGROUP_CONTROLLER_CPUSET].mountPoint))) { - if (virCgroupCpuSetInherit(parent, group) < 0) { - VIR_FREE(path); - goto cleanup; - } + if (virCgroupCpuSetInherit(parent, group) < 0) + return -1; } /* * Note that virCgroupSetMemoryUseHierarchy should always be @@ -1112,21 +1059,14 @@ virCgroupMakeGroup(virCgroupPtr parent, (i == VIR_CGROUP_CONTROLLER_MEMORY || STREQ(group->controllers[i].mountPoint, group->controllers[VIR_CGROUP_CONTROLLER_MEMORY].mountPoint))) { - if (virCgroupSetMemoryUseHierarchy(group) < 0) { - VIR_FREE(path); - goto cleanup; - } + if (virCgroupSetMemoryUseHierarchy(group) < 0) + return -1; } } - - VIR_FREE(path); } VIR_DEBUG("Done making controllers for group"); - ret = 0; - - cleanup: - return ret; + return 0; } @@ -1338,9 +1278,9 @@ virCgroupNewPartition(const char *path, virCgroupPtr *group) { int ret = -1; - char *parentPath = NULL; + VIR_AUTOFREE(char *) parentPath = NULL; + VIR_AUTOFREE(char *) newPath = NULL; virCgroupPtr parent = NULL; - char *newPath = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); @@ -1380,8 +1320,6 @@ virCgroupNewPartition(const char *path, if (ret != 0) virCgroupFree(*group); virCgroupFree(parent); - VIR_FREE(parentPath); - VIR_FREE(newPath); return ret; } @@ -1420,18 +1358,17 @@ virCgroupNewDomainPartition(virCgroupPtr partition, bool create, virCgroupPtr *group) { - int ret = -1; - char *grpname = NULL; + VIR_AUTOFREE(char *)grpname = NULL; if (virAsprintf(&grpname, "%s.libvirt-%s", name, driver) < 0) - goto cleanup; + return -1; if (virCgroupPartitionEscape(&grpname) < 0) - goto cleanup; + return -1; if (virCgroupNew(-1, grpname, partition, -1, group) < 0) - goto cleanup; + return -1; /* * Create a cgroup with memory.use_hierarchy enabled to @@ -1447,14 +1384,10 @@ virCgroupNewDomainPartition(virCgroupPtr partition, VIR_CGROUP_MEM_HIERACHY) < 0) { virCgroupRemove(*group); virCgroupFree(*group); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - VIR_FREE(grpname); - return ret; + return 0; } @@ -1476,27 +1409,26 @@ virCgroupNewThread(virCgroupPtr domain, bool create, virCgroupPtr *group) { - int ret = -1; - char *name = NULL; + VIR_AUTOFREE(char *) name = NULL; int controllers; switch (nameval) { case VIR_CGROUP_THREAD_VCPU: if (virAsprintf(&name, "vcpu%d", id) < 0) - goto cleanup; + return -1; break; case VIR_CGROUP_THREAD_EMULATOR: if (VIR_STRDUP(name, "emulator") < 0) - goto cleanup; + return -1; break; case VIR_CGROUP_THREAD_IOTHREAD: if (virAsprintf(&name, "iothread%d", id) < 0) - goto cleanup; + return -1; break; case VIR_CGROUP_THREAD_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("unexpected name value %d"), nameval); - goto cleanup; + return -1; } controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) | @@ -1504,18 +1436,15 @@ virCgroupNewThread(virCgroupPtr domain, (1 << VIR_CGROUP_CONTROLLER_CPUSET)); if (virCgroupNew(-1, name, domain, controllers, group) < 0) - goto cleanup; + return -1; if (virCgroupMakeGroup(domain, *group, create, VIR_CGROUP_NONE) < 0) { virCgroupRemove(*group); virCgroupFree(*group); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(name); - return ret; + return 0; } @@ -1576,7 +1505,7 @@ virCgroupNewMachineSystemd(const char *name, int ret = -1; int rv; virCgroupPtr init, parent = NULL; - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; char *offset; VIR_DEBUG("Trying to setup machine '%s' via systemd", name); @@ -1661,7 +1590,6 @@ virCgroupNewMachineSystemd(const char *name, ret = 0; cleanup: virCgroupFree(parent); - VIR_FREE(path); return ret; } @@ -1893,9 +1821,11 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, long long *requests_write) { long long stats_val; - char *str1 = NULL, *str2 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + char *p1; + char *p2; size_t i; - int ret = -1; const char *value_names[] = { "Read ", @@ -1918,12 +1848,12 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_service_bytes", &str1) < 0) - goto cleanup; + return -1; if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_serviced", &str2) < 0) - goto cleanup; + return -1; /* sum up all entries of the same kind, from all devices */ for (i = 0; i < ARRAY_CARDINALITY(value_names); i++) { @@ -1937,7 +1867,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, _("Cannot parse byte %sstat '%s'"), value_names[i], p1); - goto cleanup; + return -1; } if (stats_val < 0 || @@ -1946,7 +1876,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, virReportError(VIR_ERR_OVERFLOW, _("Sum of byte %sstat overflows"), value_names[i]); - goto cleanup; + return -1; } *bytes_ptrs[i] += stats_val; } @@ -1958,7 +1888,7 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, _("Cannot parse %srequest stat '%s'"), value_names[i], p2); - goto cleanup; + return -1; } if (stats_val < 0 || @@ -1967,18 +1897,13 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, virReportError(VIR_ERR_OVERFLOW, _("Sum of %srequest stat overflows"), value_names[i]); - goto cleanup; + return -1; } *requests_ptrs[i] += stats_val; } } - ret = 0; - - cleanup: - VIR_FREE(str2); - VIR_FREE(str1); - return ret; + return 0; } @@ -2002,9 +1927,12 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, long long *requests_read, long long *requests_write) { - char *str1 = NULL, *str2 = NULL, *str3 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + VIR_AUTOFREE(char *) str3 = NULL; + char *p1; + char *p2; size_t i; - int ret = -1; const char *value_names[] = { "Read ", @@ -2022,28 +1950,28 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_service_bytes", &str1) < 0) - goto cleanup; + return -1; if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.io_serviced", &str2) < 0) - goto cleanup; + return -1; if (!(str3 = virCgroupGetBlockDevString(path))) - goto cleanup; + return -1; if (!(p1 = strstr(str1, str3))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find byte stats for block device '%s'"), str3); - goto cleanup; + return -1; } if (!(p2 = strstr(str2, str3))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find request stats for block device '%s'"), str3); - goto cleanup; + return -1; } for (i = 0; i < ARRAY_CARDINALITY(value_names); i++) { @@ -2051,38 +1979,32 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find byte %sstats for block device '%s'"), value_names[i], str3); - goto cleanup; + return -1; } if (virStrToLong_ll(p1 + strlen(value_names[i]), &p1, 10, bytes_ptrs[i]) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse %sstat '%s'"), value_names[i], p1 + strlen(value_names[i])); - goto cleanup; + return -1; } if (!(p2 = strstr(p2, value_names[i]))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot find request %sstats for block device '%s'"), value_names[i], str3); - goto cleanup; + return -1; } if (virStrToLong_ll(p2 + strlen(value_names[i]), &p2, 10, requests_ptrs[i]) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse %sstat '%s'"), value_names[i], p2 + strlen(value_names[i])); - goto cleanup; + return -1; } } - ret = 0; - - cleanup: - VIR_FREE(str3); - VIR_FREE(str2); - VIR_FREE(str1); - return ret; + return 0; } @@ -2138,24 +2060,19 @@ virCgroupSetBlkioDeviceReadIops(virCgroupPtr group, const char *path, unsigned int riops) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%u", blkstr, riops) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_iops_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } @@ -2172,24 +2089,19 @@ virCgroupSetBlkioDeviceWriteIops(virCgroupPtr group, const char *path, unsigned int wiops) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%u", blkstr, wiops) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_iops_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } @@ -2206,24 +2118,19 @@ virCgroupSetBlkioDeviceReadBps(virCgroupPtr group, const char *path, unsigned long long rbps) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%llu", blkstr, rbps) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_bps_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } /** @@ -2239,24 +2146,19 @@ virCgroupSetBlkioDeviceWriteBps(virCgroupPtr group, const char *path, unsigned long long wbps) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%llu", blkstr, wbps) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_bps_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } @@ -2274,24 +2176,19 @@ virCgroupSetBlkioDeviceWeight(virCgroupPtr group, const char *path, unsigned int weight) { - char *str = NULL; - char *blkstr = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; + VIR_AUTOFREE(char *) blkstr = NULL; if (!(blkstr = virCgroupGetBlockDevString(path))) return -1; if (virAsprintf(&str, "%s%d", blkstr, weight) < 0) - goto error; + return -1; - ret = virCgroupSetValueStr(group, + return virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.weight_device", str); - error: - VIR_FREE(blkstr); - VIR_FREE(str); - return ret; } /** @@ -2307,15 +2204,14 @@ virCgroupGetBlkioDeviceReadIops(virCgroupPtr group, const char *path, unsigned int *riops) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_iops_device", path, &str) < 0) - goto error; + return -1; if (!str) { *riops = 0; @@ -2323,13 +2219,10 @@ virCgroupGetBlkioDeviceReadIops(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2345,15 +2238,14 @@ virCgroupGetBlkioDeviceWriteIops(virCgroupPtr group, const char *path, unsigned int *wiops) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_iops_device", path, &str) < 0) - goto error; + return -1; if (!str) { *wiops = 0; @@ -2361,13 +2253,10 @@ virCgroupGetBlkioDeviceWriteIops(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2383,15 +2272,14 @@ virCgroupGetBlkioDeviceReadBps(virCgroupPtr group, const char *path, unsigned long long *rbps) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.read_bps_device", path, &str) < 0) - goto error; + return -1; if (!str) { *rbps = 0; @@ -2399,13 +2287,10 @@ virCgroupGetBlkioDeviceReadBps(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2421,15 +2306,14 @@ virCgroupGetBlkioDeviceWriteBps(virCgroupPtr group, const char *path, unsigned long long *wbps) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.throttle.write_bps_device", path, &str) < 0) - goto error; + return -1; if (!str) { *wbps = 0; @@ -2437,13 +2321,10 @@ virCgroupGetBlkioDeviceWriteBps(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } /** @@ -2459,15 +2340,14 @@ virCgroupGetBlkioDeviceWeight(virCgroupPtr group, const char *path, unsigned int *weight) { - char *str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) str = NULL; if (virCgroupGetValueForBlkDev(group, VIR_CGROUP_CONTROLLER_BLKIO, "blkio.weight_device", path, &str) < 0) - goto error; + return -1; if (!str) { *weight = 0; @@ -2475,13 +2355,10 @@ virCgroupGetBlkioDeviceWeight(virCgroupPtr group, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to parse '%s' as an integer"), str); - goto error; + return -1; } - ret = 0; - error: - VIR_FREE(str); - return ret; + return 0; } @@ -2940,36 +2817,29 @@ int virCgroupAllowDevice(virCgroupPtr group, char type, int major, int minor, int perms) { - int ret = -1; - char *devstr = NULL; - char *majorstr = NULL; - char *minorstr = NULL; + VIR_AUTOFREE(char *) devstr = NULL; + VIR_AUTOFREE(char *) majorstr = NULL; + VIR_AUTOFREE(char *) minorstr = NULL; if ((major < 0 && VIR_STRDUP(majorstr, "*") < 0) || (major >= 0 && virAsprintf(&majorstr, "%i", major) < 0)) - goto cleanup; + return -1; if ((minor < 0 && VIR_STRDUP(minorstr, "*") < 0) || (minor >= 0 && virAsprintf(&minorstr, "%i", minor) < 0)) - goto cleanup; + return -1; if (virAsprintf(&devstr, "%c %s:%s %s", type, majorstr, minorstr, virCgroupGetDevicePermsString(perms)) < 0) - goto cleanup; + return -1; if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_DEVICES, "devices.allow", devstr) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - VIR_FREE(devstr); - VIR_FREE(majorstr); - VIR_FREE(minorstr); - return ret; + return 0; } @@ -3031,36 +2901,29 @@ int virCgroupDenyDevice(virCgroupPtr group, char type, int major, int minor, int perms) { - int ret = -1; - char *devstr = NULL; - char *majorstr = NULL; - char *minorstr = NULL; + VIR_AUTOFREE(char *) devstr = NULL; + VIR_AUTOFREE(char *) majorstr = NULL; + VIR_AUTOFREE(char *) minorstr = NULL; if ((major < 0 && VIR_STRDUP(majorstr, "*") < 0) || (major >= 0 && virAsprintf(&majorstr, "%i", major) < 0)) - goto cleanup; + return -1; if ((minor < 0 && VIR_STRDUP(minorstr, "*") < 0) || (minor >= 0 && virAsprintf(&minorstr, "%i", minor) < 0)) - goto cleanup; + return -1; if (virAsprintf(&devstr, "%c %s:%s %s", type, majorstr, minorstr, virCgroupGetDevicePermsString(perms)) < 0) - goto cleanup; + return -1; if (virCgroupSetValueStr(group, VIR_CGROUP_CONTROLLER_DEVICES, "devices.deny", devstr) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - VIR_FREE(devstr); - VIR_FREE(majorstr); - VIR_FREE(minorstr); - return ret; + return 0; } @@ -3130,10 +2993,10 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, { int ret = -1; ssize_t i = -1; - char *buf = NULL; virCgroupPtr group_vcpu = NULL; while ((i = virBitmapNextSetBit(guestvcpus, i)) >= 0) { + VIR_AUTOFREE(char *) buf = NULL; char *pos; unsigned long long tmp; ssize_t j; @@ -3158,13 +3021,11 @@ virCgroupGetPercpuVcpuSum(virCgroupPtr group, } virCgroupFree(group_vcpu); - VIR_FREE(buf); } ret = 0; cleanup: virCgroupFree(group_vcpu); - VIR_FREE(buf); return ret; } @@ -3201,8 +3062,8 @@ virCgroupGetPercpuStats(virCgroupPtr group, size_t i; int need_cpus, total_cpus; char *pos; - char *buf = NULL; - unsigned long long *sum_cpu_time = NULL; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOFREE(unsigned long long *) sum_cpu_time = NULL; virTypedParameterPtr ent; int param_idx; unsigned long long cpu_time; @@ -3288,8 +3149,6 @@ virCgroupGetPercpuStats(virCgroupPtr group, cleanup: virBitmapFree(cpumap); - VIR_FREE(sum_cpu_time); - VIR_FREE(buf); return ret; } @@ -3460,7 +3319,7 @@ virCgroupRemoveRecursively(char *grppath) /* This is best-effort cleanup: we want to log failures with just * VIR_ERROR instead of normal virReportError */ while ((direrr = virDirRead(grpdir, &ent, NULL)) > 0) { - char *path; + VIR_AUTOFREE(char *) path = NULL; if (ent->d_type != DT_DIR) continue; @@ -3469,7 +3328,6 @@ virCgroupRemoveRecursively(char *grppath) break; } rc = virCgroupRemoveRecursively(path); - VIR_FREE(path); if (rc != 0) break; } @@ -3507,10 +3365,11 @@ virCgroupRemove(virCgroupPtr group) { int rc = 0; size_t i; - char *grppath = NULL; VIR_DEBUG("Removing cgroup %s", group->path); for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { + VIR_AUTOFREE(char *) grppath = NULL; + /* Skip over controllers not mounted */ if (!group->controllers[i].mountPoint) continue; @@ -3532,7 +3391,6 @@ virCgroupRemove(virCgroupPtr group) VIR_DEBUG("Removing cgroup %s and all child cgroups", grppath); rc = virCgroupRemoveRecursively(grppath); - VIR_FREE(grppath); } VIR_DEBUG("Done removing cgroup %s", group->path); @@ -3548,7 +3406,7 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) { int ret = -1; bool killedAny = false; - char *keypath = NULL; + VIR_AUTOFREE(char *) keypath = NULL; bool done = false; FILE *fp = NULL; VIR_DEBUG("group=%p path=%s signum=%d pids=%p", @@ -3612,7 +3470,6 @@ virCgroupKillInternal(virCgroupPtr group, int signum, virHashTablePtr pids) ret = killedAny ? 1 : 0; cleanup: - VIR_FREE(keypath); VIR_FORCE_FCLOSE(fp); return ret; @@ -3677,7 +3534,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, int ret = -1; int rc; bool killedAny = false; - char *keypath = NULL; + VIR_AUTOFREE(char *) keypath = NULL; DIR *dp = NULL; virCgroupPtr subgroup = NULL; struct dirent *ent; @@ -3731,7 +3588,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, cleanup: virCgroupFree(subgroup); - VIR_FREE(keypath); VIR_DIR_CLOSE(dp); return ret; } @@ -3845,9 +3701,8 @@ int virCgroupGetCpuacctStat(virCgroupPtr group, unsigned long long *user, unsigned long long *sys) { - char *str; + VIR_AUTOFREE(char *) str = NULL; char *p; - int ret = -1; static double scale = -1.0; if (virCgroupGetValueStr(group, VIR_CGROUP_CONTROLLER_CPUACCT, @@ -3859,14 +3714,14 @@ virCgroupGetCpuacctStat(virCgroupPtr group, unsigned long long *user, virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse user stat '%s'"), p); - goto cleanup; + return -1; } if (!(p = STRSKIP(p, "\nsystem ")) || virStrToLong_ull(p, NULL, 10, sys) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Cannot parse sys stat '%s'"), p); - goto cleanup; + return -1; } /* times reported are in system ticks (generally 100 Hz), but that * rate can theoretically vary between machines. Scale things @@ -3876,17 +3731,14 @@ virCgroupGetCpuacctStat(virCgroupPtr group, unsigned long long *user, if (ticks_per_sec == -1) { virReportSystemError(errno, "%s", _("Cannot determine system clock HZ")); - goto cleanup; + return -1; } scale = 1000000000.0 / ticks_per_sec; } *user *= scale; *sys *= scale; - ret = 0; - cleanup: - VIR_FREE(str); - return ret; + return 0; } @@ -3912,10 +3764,9 @@ int virCgroupBindMount(virCgroupPtr group, const char *oldroot, const char *mountopts) { - int ret = -1; size_t i; - char *opts = NULL; - char *root = NULL; + VIR_AUTOFREE(char *) opts = NULL; + VIR_AUTOFREE(char *) root = NULL; if (!(root = virCgroupIdentifyRoot(group))) return -1; @@ -3926,18 +3777,18 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, virReportSystemError(errno, _("Unable to create directory %s"), root); - goto cleanup; + return -1; } if (virAsprintf(&opts, "mode=755,size=65536%s", mountopts) < 0) - goto cleanup; + return -1; if (mount("tmpfs", root, "tmpfs", MS_NOSUID|MS_NODEV|MS_NOEXEC, opts) < 0) { virReportSystemError(errno, _("Failed to mount %s on %s type %s"), "tmpfs", root, "tmpfs"); - goto cleanup; + return -1; } for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { @@ -3945,11 +3796,11 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, continue; if (!virFileExists(group->controllers[i].mountPoint)) { - char *src; + VIR_AUTOFREE(char *) src = NULL; if (virAsprintf(&src, "%s%s", oldroot, group->controllers[i].mountPoint) < 0) - goto cleanup; + return -1; VIR_DEBUG("Create mount point '%s'", group->controllers[i].mountPoint); @@ -3957,8 +3808,7 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, virReportSystemError(errno, _("Unable to create directory %s"), group->controllers[i].mountPoint); - VIR_FREE(src); - goto cleanup; + return -1; } if (mount(src, group->controllers[i].mountPoint, "none", MS_BIND, @@ -3966,11 +3816,8 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, virReportSystemError(errno, _("Failed to bind cgroup '%s' on '%s'"), src, group->controllers[i].mountPoint); - VIR_FREE(src); - goto cleanup; + return -1; } - - VIR_FREE(src); } if (group->controllers[i].linkPoint) { @@ -3983,16 +3830,12 @@ virCgroupBindMount(virCgroupPtr group, const char *oldroot, _("Unable to symlink directory %s to %s"), group->controllers[i].mountPoint, group->controllers[i].linkPoint); - goto cleanup; + return -1; } } } - ret = 0; - cleanup: - VIR_FREE(root); - VIR_FREE(opts); - return ret; + return 0; } @@ -4003,11 +3846,11 @@ int virCgroupSetOwner(virCgroupPtr cgroup, { int ret = -1; size_t i; - char *base = NULL, *entry = NULL; DIR *dh = NULL; int direrr; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { + VIR_AUTOFREE(char *) base = NULL; struct dirent *de; if (!((1 << i) & controllers)) @@ -4024,6 +3867,8 @@ int virCgroupSetOwner(virCgroupPtr cgroup, goto cleanup; while ((direrr = virDirRead(dh, &de, base)) > 0) { + VIR_AUTOFREE(char *) entry = NULL; + if (virAsprintf(&entry, "%s/%s", base, de->d_name) < 0) goto cleanup; @@ -4033,8 +3878,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup, entry, uid, gid); goto cleanup; } - - VIR_FREE(entry); } if (direrr < 0) goto cleanup; @@ -4046,7 +3889,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup, goto cleanup; } - VIR_FREE(base); VIR_DIR_CLOSE(dh); } @@ -4054,8 +3896,6 @@ int virCgroupSetOwner(virCgroupPtr cgroup, cleanup: VIR_DIR_CLOSE(dh); - VIR_FREE(entry); - VIR_FREE(base); return ret; } @@ -4070,8 +3910,7 @@ int virCgroupSetOwner(virCgroupPtr cgroup, bool virCgroupSupportsCpuBW(virCgroupPtr cgroup) { - char *path = NULL; - bool ret = false; + VIR_AUTOFREE(char *) path = NULL; if (!cgroup) return false; @@ -4079,21 +3918,17 @@ virCgroupSupportsCpuBW(virCgroupPtr cgroup) if (virCgroupPathOfController(cgroup, VIR_CGROUP_CONTROLLER_CPU, "cpu.cfs_period_us", &path) < 0) { virResetLastError(); - goto cleanup; + return false; } - ret = virFileExists(path); - - cleanup: - VIR_FREE(path); - return ret; + return virFileExists(path); } int virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) { int ret = -1; - char *content = NULL; + VIR_AUTOFREE(char *) content = NULL; if (!cgroup) return -1; @@ -4103,7 +3938,6 @@ virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) if (ret == 0 && content[0] == '\0') ret = 1; - VIR_FREE(content); return ret; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:41PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- ...
@@ -1893,9 +1821,11 @@ virCgroupGetBlkioIoServiced(virCgroupPtr group, long long *requests_write) { long long stats_val; - char *str1 = NULL, *str2 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + char *p1; + char *p2;
Could have initialized ^these 2 as well for that matter... ...
@@ -2002,9 +1927,12 @@ virCgroupGetBlkioIoDeviceServiced(virCgroupPtr group, long long *requests_read, long long *requests_write) { - char *str1 = NULL, *str2 = NULL, *str3 = NULL, *p1, *p2; + VIR_AUTOFREE(char *) str1 = NULL; + VIR_AUTOFREE(char *) str2 = NULL; + VIR_AUTOFREE(char *) str3 = NULL; + char *p1; + char *p2;
...here too... Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/vircgroup.c | 155 +++++++++++++++++---------------------------------- 1 file changed, 52 insertions(+), 103 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 4bb4408..cdb493e 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -836,25 +836,21 @@ virCgroupGetValueForBlkDev(virCgroupPtr group, { VIR_AUTOFREE(char *) prefix = NULL; VIR_AUTOFREE(char *) str = NULL; - char **lines = NULL; - int ret = -1; + VIR_AUTOPTR(virString) lines = NULL; if (virCgroupGetValueStr(group, controller, key, &str) < 0) - goto error; + return -1; if (!(prefix = virCgroupGetBlockDevString(path))) - goto error; + return -1; if (!(lines = virStringSplit(str, "\n", -1))) - goto error; + return -1; if (VIR_STRDUP(*value, virStringListGetFirstWithPrefix(lines, prefix)) < 0) - goto error; + return -1; - ret = 0; - error: - virStringListFree(lines); - return ret; + return 0; } @@ -1217,12 +1213,11 @@ virCgroupAddTaskController(virCgroupPtr group, pid_t pid, int controller) static int virCgroupSetPartitionSuffix(const char *path, char **res) { - char **tokens; + VIR_AUTOPTR(virString) tokens = NULL; size_t i; - int ret = -1; if (!(tokens = virStringSplit(path, "/", 0))) - return ret; + return -1; for (i = 0; tokens[i] != NULL; i++) { /* Whitelist the 3 top level fixed dirs @@ -1241,22 +1236,18 @@ virCgroupSetPartitionSuffix(const char *path, char **res) !strchr(tokens[i], '.')) { if (VIR_REALLOC_N(tokens[i], strlen(tokens[i]) + strlen(".partition") + 1) < 0) - goto cleanup; + return -1; strcat(tokens[i], ".partition"); } if (virCgroupPartitionEscape(&(tokens[i])) < 0) - goto cleanup; + return -1; } if (!(*res = virStringListJoin((const char **)tokens, "/"))) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virStringListFree(tokens); - return ret; + return 0; } @@ -1280,7 +1271,7 @@ virCgroupNewPartition(const char *path, int ret = -1; VIR_AUTOFREE(char *) parentPath = NULL; VIR_AUTOFREE(char *) newPath = NULL; - virCgroupPtr parent = NULL; + VIR_AUTOPTR(virCgroup) parent = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); @@ -1319,7 +1310,6 @@ virCgroupNewPartition(const char *path, cleanup: if (ret != 0) virCgroupFree(*group); - virCgroupFree(parent); return ret; } @@ -1502,9 +1492,9 @@ virCgroupNewMachineSystemd(const char *name, int controllers, virCgroupPtr *group) { - int ret = -1; int rv; - virCgroupPtr init, parent = NULL; + VIR_AUTOPTR(virCgroup) init = NULL; + VIR_AUTOPTR(virCgroup) parent = NULL; VIR_AUTOFREE(char *) path = NULL; char *offset; @@ -1531,12 +1521,10 @@ virCgroupNewMachineSystemd(const char *name, path = init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement; init->controllers[VIR_CGROUP_CONTROLLER_SYSTEMD].placement = NULL; - virCgroupFree(init); if (!path || STREQ(path, "/") || path[0] != '/') { VIR_DEBUG("Systemd didn't setup its controller"); - ret = -2; - goto cleanup; + return -2; } offset = path; @@ -1546,7 +1534,7 @@ virCgroupNewMachineSystemd(const char *name, NULL, controllers, &parent) < 0) - goto cleanup; + return -1; for (;;) { @@ -1560,11 +1548,11 @@ virCgroupNewMachineSystemd(const char *name, parent, controllers, &tmp) < 0) - goto cleanup; + return -1; if (virCgroupMakeGroup(parent, tmp, true, VIR_CGROUP_NONE) < 0) { virCgroupFree(tmp); - goto cleanup; + return -1; } if (t) { *t = '/'; @@ -1587,10 +1575,7 @@ virCgroupNewMachineSystemd(const char *name, } } - ret = 0; - cleanup: - virCgroupFree(parent); - return ret; + return 0; } @@ -1611,8 +1596,7 @@ virCgroupNewMachineManual(const char *name, int controllers, virCgroupPtr *group) { - virCgroupPtr parent = NULL; - int ret = -1; + VIR_AUTOPTR(virCgroup) parent = NULL; VIR_DEBUG("Fallback to non-systemd setup"); if (virCgroupNewPartition(partition, @@ -1620,9 +1604,9 @@ virCgroupNewMachineManual(const char *name, controllers, &parent) < 0) { if (virCgroupNewIgnoreError()) - goto done; + return 0; - goto cleanup; + return -1; } if (virCgroupNewDomainPartition(parent, @@ -1630,7 +1614,7 @@ virCgroupNewMachineManual(const char *name, name, true, group) < 0) - goto cleanup; + return -1; if (virCgroupAddTask(*group, pidleader) < 0) { virErrorPtr saved = virSaveLastError(); @@ -1642,12 +1626,7 @@ virCgroupNewMachineManual(const char *name, } } - done: - ret = 0; - - cleanup: - virCgroupFree(parent); - return ret; + return 0; } @@ -2376,7 +2355,7 @@ static virOnceControl virCgroupMemoryOnce = VIR_ONCE_CONTROL_INITIALIZER; static void virCgroupMemoryOnceInit(void) { - virCgroupPtr group; + VIR_AUTOPTR(virCgroup) group = NULL; unsigned long long int mem_unlimited = 0ULL; if (virCgroupNew(-1, "/", NULL, -1, &group) < 0) @@ -2390,7 +2369,6 @@ virCgroupMemoryOnceInit(void) "memory.limit_in_bytes", &mem_unlimited)); cleanup: - virCgroupFree(group); virCgroupMemoryUnlimitedKB = mem_unlimited >> 10; } @@ -2991,22 +2969,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) { VIR_AUTOFREE(char *) buf = NULL; + VIR_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); @@ -3015,18 +2992,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; } @@ -3058,7 +3030,6 @@ virCgroupGetPercpuStats(virCgroupPtr group, unsigned int ncpus, virBitmapPtr guestvcpus) { - int ret = -1; size_t i; int need_cpus, total_cpus; char *pos; @@ -3067,7 +3038,7 @@ virCgroupGetPercpuStats(virCgroupPtr group, virTypedParameterPtr ent; int param_idx; unsigned long long cpu_time; - virBitmapPtr cpumap = NULL; + VIR_AUTOPTR(virBitmap) cpumap = NULL; /* return the number of supported params */ if (nparams == 0 && ncpus != 0) { @@ -3084,21 +3055,19 @@ virCgroupGetPercpuStats(virCgroupPtr group, total_cpus = virBitmapSize(cpumap); /* return total number of cpus */ - if (ncpus == 0) { - ret = total_cpus; - goto cleanup; - } + if (ncpus == 0) + return total_cpus; if (start_cpu >= total_cpus) { virReportError(VIR_ERR_INVALID_ARG, _("start_cpu %d larger than maximum of %d"), start_cpu, total_cpus - 1); - goto cleanup; + return -1; } /* we get percpu cputime accounting info. */ if (virCgroupGetCpuacctPercpuUsage(group, &buf)) - goto cleanup; + return -1; pos = buf; /* return percpu cputime in index 0 */ @@ -3113,14 +3082,14 @@ virCgroupGetPercpuStats(virCgroupPtr group, } else if (virStrToLong_ull(pos, &pos, 10, &cpu_time) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("cpuacct parse error")); - goto cleanup; + return -1; } if (i < start_cpu) continue; ent = ¶ms[(i - start_cpu) * nparams + param_idx]; if (virTypedParameterAssign(ent, VIR_DOMAIN_CPU_STATS_CPUTIME, VIR_TYPED_PARAM_ULLONG, cpu_time) < 0) - goto cleanup; + return -1; } /* return percpu vcputime in index 1 */ @@ -3128,10 +3097,10 @@ virCgroupGetPercpuStats(virCgroupPtr group, if (guestvcpus && param_idx < nparams) { if (VIR_ALLOC_N(sum_cpu_time, need_cpus) < 0) - goto cleanup; + return -1; if (virCgroupGetPercpuVcpuSum(group, guestvcpus, sum_cpu_time, need_cpus, cpumap) < 0) - goto cleanup; + return -1; for (i = start_cpu; i < need_cpus; i++) { if (virTypedParameterAssign(¶ms[(i - start_cpu) * nparams + @@ -3139,17 +3108,13 @@ virCgroupGetPercpuStats(virCgroupPtr group, VIR_DOMAIN_CPU_STATS_VCPUTIME, VIR_TYPED_PARAM_ULLONG, sum_cpu_time[i]) < 0) - goto cleanup; + return -1; } param_idx++; } - ret = param_idx; - - cleanup: - virBitmapFree(cpumap); - return ret; + return param_idx; } @@ -3505,23 +3470,18 @@ int virCgroupKill(virCgroupPtr group, int signum) { VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); - int ret; /* The 'tasks' file in cgroups can contain duplicated * pids, so we use a hash to track which we've already * killed. */ - virHashTablePtr pids = virHashCreateFull(100, + VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100, NULL, virCgroupPidCode, virCgroupPidEqual, virCgroupPidCopy, NULL); - ret = virCgroupKillInternal(group, signum, pids); - - virHashFree(pids); - - return ret; + return virCgroupKillInternal(group, signum, pids); } @@ -3536,7 +3496,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, bool killedAny = false; VIR_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", @@ -3561,6 +3520,8 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, } while ((direrr = virDirRead(dp, &ent, keypath)) > 0) { + VIR_AUTOPTR(virCgroup) subgroup = NULL; + if (ent->d_type != DT_DIR) continue; @@ -3577,8 +3538,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if (dormdir) virCgroupRemove(subgroup); - - virCgroupFree(subgroup); } if (direrr < 0) goto cleanup; @@ -3587,7 +3546,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0; cleanup: - virCgroupFree(subgroup); VIR_DIR_CLOSE(dp); return ret; } @@ -3596,20 +3554,15 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, int virCgroupKillRecursive(virCgroupPtr group, int signum) { - int ret; VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); - virHashTablePtr pids = virHashCreateFull(100, + VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100, NULL, virCgroupPidCode, virCgroupPidEqual, virCgroupPidCopy, NULL); - ret = virCgroupKillRecursiveInternal(group, signum, pids, false); - - virHashFree(pids); - - return ret; + return virCgroupKillRecursiveInternal(group, signum, pids, false); } @@ -3944,15 +3897,12 @@ virCgroupHasEmptyTasks(virCgroupPtr cgroup, int controller) bool virCgroupControllerAvailable(int controller) { - virCgroupPtr cgroup; - bool ret = false; + VIR_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 /* !VIR_CGROUP_SUPPORTED */ @@ -4740,7 +4690,7 @@ virCgroupDelThread(virCgroupPtr cgroup, virCgroupThreadName nameval, int idx) { - virCgroupPtr new_cgroup = NULL; + VIR_AUTOPTR(virCgroup) new_cgroup = NULL; if (cgroup) { if (virCgroupNewThread(cgroup, nameval, idx, false, &new_cgroup) < 0) @@ -4748,7 +4698,6 @@ virCgroupDelThread(virCgroupPtr cgroup, /* Remove the offlined cgroup */ virCgroupRemove(new_cgroup); - virCgroupFree(new_cgroup); } return 0; -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:42PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- ...
@@ -3505,23 +3470,18 @@ int virCgroupKill(virCgroupPtr group, int signum) { VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); - int ret; /* The 'tasks' file in cgroups can contain duplicated * pids, so we use a hash to track which we've already * killed. */ - virHashTablePtr pids = virHashCreateFull(100, + VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100, NULL, virCgroupPidCode, virCgroupPidEqual, virCgroupPidCopy, NULL);
Code misalignment... ...
@@ -3596,20 +3554,15 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, int virCgroupKillRecursive(virCgroupPtr group, int signum) { - int ret; VIR_DEBUG("group=%p path=%s signum=%d", group, group->path, signum); - virHashTablePtr pids = virHashCreateFull(100, + VIR_AUTOPTR(virHashTable) pids = virHashCreateFull(100, NULL, virCgroupPidCode, virCgroupPidEqual, virCgroupPidCopy, NULL);
...here too... Reviewed-by: Erik Skultety <eskultet@redhat.com>
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When variables of type virMediatedDevicePtr and virMediatedDeviceTypePtr are declared using VIR_AUTOPTR, the functions virMediatedDeviceFree and virMediatedDeviceTypeFree, respectively, will be run automatically on them when they go out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virmdev.c | 1 - src/util/virmdev.h | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 6c51388..d7bcb1d 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -21,7 +21,6 @@ #include "dirname.h" #include "virmdev.h" #include "virlog.h" -#include "viralloc.h" #include "virerror.h" #include "virfile.h" #include "virstring.h" diff --git a/src/util/virmdev.h b/src/util/virmdev.h index cfda2ca..7c93c4d 100644 --- a/src/util/virmdev.h +++ b/src/util/virmdev.h @@ -22,6 +22,7 @@ # include "internal.h" # include "virobject.h" # include "virutil.h" +# include "viralloc.h" typedef enum { VIR_MDEV_MODEL_TYPE_VFIO_PCI = 0, @@ -135,4 +136,7 @@ int virMediatedDeviceTypeReadAttrs(const char *sysfspath, virMediatedDeviceTypePtr *type); +VIR_DEFINE_AUTOPTR_FUNC(virMediatedDevice, virMediatedDeviceFree) +VIR_DEFINE_AUTOPTR_FUNC(virMediatedDeviceType, virMediatedDeviceTypeFree) + #endif /* __VIR_MDEV_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:43PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When variables of type virMediatedDevicePtr and virMediatedDeviceTypePtr are declared using VIR_AUTOPTR, the functions virMediatedDeviceFree and virMediatedDeviceTypeFree, respectively, will be run automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virmdev.c | 53 +++++++++++++++++++---------------------------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index d7bcb1d..04c9302 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -72,24 +72,23 @@ static int virMediatedDeviceGetSysfsDeviceAPI(virMediatedDevicePtr dev, char **device_api) { - int ret = -1; - char *buf = NULL; - char *file = NULL; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOFREE(char *) file = NULL; char *tmp = NULL; if (virAsprintf(&file, "%s/mdev_type/device_api", dev->path) < 0) - goto cleanup; + return -1; /* TODO - make this a generic method to access sysfs files for various * kinds of devices */ if (!virFileExists(file)) { virReportSystemError(errno, _("failed to read '%s'"), file); - goto cleanup; + return -1; } if (virFileReadAll(file, 1024, &buf) < 0) - goto cleanup; + return -1; if ((tmp = strchr(buf, '\n'))) *tmp = '\0'; @@ -97,11 +96,7 @@ virMediatedDeviceGetSysfsDeviceAPI(virMediatedDevicePtr dev, *device_api = buf; buf = NULL; - ret = 0; - cleanup: - VIR_FREE(file); - VIR_FREE(buf); - return ret; + return 0; } @@ -109,8 +104,7 @@ static int virMediatedDeviceCheckModel(virMediatedDevicePtr dev, virMediatedDeviceModelType model) { - int ret = -1; - char *dev_api = NULL; + VIR_AUTOFREE(char *) dev_api = NULL; int actual_model; if (virMediatedDeviceGetSysfsDeviceAPI(dev, &dev_api) < 0) @@ -123,7 +117,7 @@ virMediatedDeviceCheckModel(virMediatedDevicePtr dev, virReportError(VIR_ERR_INTERNAL_ERROR, _("device API '%s' not supported yet"), dev_api); - goto cleanup; + return -1; } if (actual_model != model) { @@ -132,13 +126,10 @@ virMediatedDeviceCheckModel(virMediatedDevicePtr dev, "device only supports '%s'"), virMediatedDeviceModelTypeToString(model), dev->path, dev_api); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(dev_api); - return ret; + return 0; } @@ -147,7 +138,7 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) { virMediatedDevicePtr ret = NULL; virMediatedDevicePtr dev = NULL; - char *sysfspath = NULL; + VIR_AUTOFREE(char *) sysfspath = NULL; if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr))) goto cleanup; @@ -173,7 +164,6 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) VIR_STEAL_PTR(ret, dev); cleanup: - VIR_FREE(sysfspath); virMediatedDeviceFree(dev); return ret; } @@ -218,34 +208,30 @@ virMediatedDeviceGetPath(virMediatedDevicePtr dev) char * virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr) { - char *result_path = NULL; - char *iommu_path = NULL; + VIR_AUTOFREE(char *) result_path = NULL; + VIR_AUTOFREE(char *) iommu_path = NULL; + VIR_AUTOFREE(char *) dev_path = virMediatedDeviceGetSysfsPath(uuidstr); char *vfio_path = NULL; - char *dev_path = virMediatedDeviceGetSysfsPath(uuidstr); if (!dev_path) return NULL; if (virAsprintf(&iommu_path, "%s/iommu_group", dev_path) < 0) - goto cleanup; + return NULL; if (!virFileExists(iommu_path)) { virReportSystemError(errno, _("failed to access '%s'"), iommu_path); - goto cleanup; + return NULL; } if (virFileResolveLink(iommu_path, &result_path) < 0) { virReportSystemError(errno, _("failed to resolve '%s'"), iommu_path); - goto cleanup; + return NULL; } if (virAsprintf(&vfio_path, "/dev/vfio/%s", last_component(result_path)) < 0) - goto cleanup; + return vfio_path; - cleanup: - VIR_FREE(result_path); - VIR_FREE(iommu_path); - VIR_FREE(dev_path); return vfio_path; } @@ -253,7 +239,7 @@ virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr) int virMediatedDeviceGetIOMMUGroupNum(const char *uuidstr) { - char *vfio_path = NULL; + VIR_AUTOFREE(char *) vfio_path = NULL; char *group_num_str = NULL; unsigned int group_num = -1; @@ -263,7 +249,6 @@ virMediatedDeviceGetIOMMUGroupNum(const char *uuidstr) group_num_str = last_component(vfio_path); ignore_value(virStrToLong_ui(group_num_str, NULL, 10, &group_num)); - VIR_FREE(vfio_path); return group_num; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:44PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- ...
@@ -218,34 +208,30 @@ virMediatedDeviceGetPath(virMediatedDevicePtr dev) char * virMediatedDeviceGetIOMMUGroupDev(const char *uuidstr) { - char *result_path = NULL; - char *iommu_path = NULL; + VIR_AUTOFREE(char *) result_path = NULL; + VIR_AUTOFREE(char *) iommu_path = NULL; + VIR_AUTOFREE(char *) dev_path = virMediatedDeviceGetSysfsPath(uuidstr); char *vfio_path = NULL; - char *dev_path = virMediatedDeviceGetSysfsPath(uuidstr);
if (!dev_path) return NULL;
if (virAsprintf(&iommu_path, "%s/iommu_group", dev_path) < 0) - goto cleanup; + return NULL;
if (!virFileExists(iommu_path)) { virReportSystemError(errno, _("failed to access '%s'"), iommu_path); - goto cleanup; + return NULL; }
if (virFileResolveLink(iommu_path, &result_path) < 0) { virReportSystemError(errno, _("failed to resolve '%s'"), iommu_path); - goto cleanup; + return NULL; }
if (virAsprintf(&vfio_path, "/dev/vfio/%s", last_component(result_path)) < 0) - goto cleanup; + return vfio_path;
I'd rather you returned NULL ^here. With that: Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virmdev.c | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/util/virmdev.c b/src/util/virmdev.c index 04c9302..46999bb 100644 --- a/src/util/virmdev.c +++ b/src/util/virmdev.c @@ -137,20 +137,20 @@ virMediatedDevicePtr virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) { virMediatedDevicePtr ret = NULL; - virMediatedDevicePtr dev = NULL; + VIR_AUTOPTR(virMediatedDevice) dev = NULL; VIR_AUTOFREE(char *) sysfspath = NULL; if (!(sysfspath = virMediatedDeviceGetSysfsPath(uuidstr))) - goto cleanup; + return NULL; if (!virFileExists(sysfspath)) { virReportError(VIR_ERR_DEVICE_MISSING, _("mediated device '%s' not found"), uuidstr); - goto cleanup; + return NULL; } if (VIR_ALLOC(dev) < 0) - goto cleanup; + return NULL; VIR_STEAL_PTR(dev->path, sysfspath); @@ -158,13 +158,11 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model) * supported mediated device's API. */ if (virMediatedDeviceCheckModel(dev, model)) - goto cleanup; + return NULL; dev->model = model; VIR_STEAL_PTR(ret, dev); - cleanup: - virMediatedDeviceFree(dev); return ret; } @@ -372,8 +370,7 @@ void virMediatedDeviceListDel(virMediatedDeviceListPtr list, virMediatedDevicePtr dev) { - virMediatedDevicePtr ret = virMediatedDeviceListSteal(list, dev); - virMediatedDeviceFree(ret); + VIR_AUTOPTR(virMediatedDevice) ret = virMediatedDeviceListSteal(list, dev); } @@ -494,23 +491,22 @@ int virMediatedDeviceTypeReadAttrs(const char *sysfspath, virMediatedDeviceTypePtr *type) { - int ret = -1; - virMediatedDeviceTypePtr tmp = NULL; + VIR_AUTOPTR(virMediatedDeviceType) tmp = NULL; #define MDEV_GET_SYSFS_ATTR(attr, dst, cb, optional) \ do { \ int rc; \ if ((rc = cb(dst, "%s/%s", sysfspath, attr)) < 0) { \ if (rc != -2 || !optional) \ - goto cleanup; \ + return 0; \ } \ } while (0) if (VIR_ALLOC(tmp) < 0) - goto cleanup; + return -1; if (VIR_STRDUP(tmp->id, last_component(sysfspath)) < 0) - goto cleanup; + return -1; /* @name sysfs attribute is optional, so getting ENOENT is fine */ MDEV_GET_SYSFS_ATTR("name", &tmp->name, virFileReadValueString, true); @@ -522,8 +518,6 @@ virMediatedDeviceTypeReadAttrs(const char *sysfspath, #undef MDEV_GET_SYSFS_ATTR VIR_STEAL_PTR(*type, tmp); - ret = 0; - cleanup: - virMediatedDeviceTypeFree(tmp); - return ret; + + return 0; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:45PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- ...
@@ -494,23 +491,22 @@ int virMediatedDeviceTypeReadAttrs(const char *sysfspath, virMediatedDeviceTypePtr *type) { - int ret = -1; - virMediatedDeviceTypePtr tmp = NULL; + VIR_AUTOPTR(virMediatedDeviceType) tmp = NULL;
#define MDEV_GET_SYSFS_ATTR(attr, dst, cb, optional) \ do { \ int rc; \ if ((rc = cb(dst, "%s/%s", sysfspath, attr)) < 0) { \ if (rc != -2 || !optional) \ - goto cleanup; \ + return 0; \
Why are you returning success ^here? With that changed: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virFirewallPtr is declared using VIR_AUTOPTR, the function virFirewallFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 1 - src/util/virfirewall.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index 10c370a..dfd792f 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -27,7 +27,6 @@ #include <stdarg.h> -#include "viralloc.h" #include "virfirewallpriv.h" #include "virerror.h" #include "virutil.h" diff --git a/src/util/virfirewall.h b/src/util/virfirewall.h index b04ab48..e024e88 100644 --- a/src/util/virfirewall.h +++ b/src/util/virfirewall.h @@ -25,6 +25,7 @@ # define __VIR_FIREWALL_H__ # include "internal.h" +# include "viralloc.h" typedef struct _virFirewall virFirewall; typedef virFirewall *virFirewallPtr; @@ -116,4 +117,6 @@ int virFirewallApply(virFirewallPtr firewall); void virFirewallSetLockOverride(bool avoid); +VIR_DEFINE_AUTOPTR_FUNC(virFirewall, virFirewallFree) + #endif /* __VIR_FIREWALL_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:46PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When a variable of type virFirewallPtr is declared using VIR_AUTOPTR, the function virFirewallFree will be run automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index dfd792f..b4a4d06 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list; VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, va_end(list); - VIR_FREE(arg); return; no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg); } @@ -678,7 +676,7 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, virCommandPtr cmd = NULL; int status; int ret = -1; - char *error = NULL; + VIR_AUTOFREE(char *) error = NULL; if (!bin) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -702,11 +700,10 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, if (ignoreErrors) { VIR_DEBUG("Ignoring error running command"); } else { - char *args = virCommandToString(cmd); + VIR_AUTOFREE(char *) args = virCommandToString(cmd); virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to apply firewall rules %s: %s"), NULLSTR(args), NULLSTR(error)); - VIR_FREE(args); VIR_FREE(*output); goto cleanup; } @@ -714,7 +711,6 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, ret = 0; cleanup: - VIR_FREE(error); virCommandFree(cmd); return ret; } @@ -807,12 +803,11 @@ virFirewallApplyRule(virFirewallPtr firewall, virFirewallRulePtr rule, bool ignoreErrors) { - char *output = NULL; + VIR_AUTOFREE(char *) output = NULL; + VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule); char **lines = NULL; int ret = -1; - char *str = virFirewallRuleToString(rule); VIR_INFO("Applying rule '%s'", NULLSTR(str)); - VIR_FREE(str); if (rule->ignoreErrors) ignoreErrors = rule->ignoreErrors; @@ -857,7 +852,6 @@ virFirewallApplyRule(virFirewallPtr firewall, ret = 0; cleanup: virStringListFree(lines); - VIR_FREE(output); return ret; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index dfd792f..b4a4d06 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list;
VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
va_end(list);
- VIR_FREE(arg); return;
no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg);
There could be an additional patch replacing the no_memory label with 'cleanup' with the obvious adjustments. With an additional patch in place: Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Mon, 23 Jul 2018 at 18:39, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index dfd792f..b4a4d06 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list;
VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
va_end(list);
- VIR_FREE(arg); return;
no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg);
There could be an additional patch replacing the no_memory label with 'cleanup' with the obvious adjustments.
There are many functions in virfirewall.c where no_memory is used instead of cleanup. So I should change either all of them, or none of them.
On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
On Mon, 23 Jul 2018 at 18:39, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index dfd792f..b4a4d06 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list;
VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
va_end(list);
- VIR_FREE(arg); return;
no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg);
There could be an additional patch replacing the no_memory label with 'cleanup' with the obvious adjustments.
There are many functions in virfirewall.c where no_memory is used instead of cleanup. So I should change either all of them, or none of them.
Yep, so it should be all of them. Erik
On Tue, 24 Jul 2018 at 11:54, Erik Skultety <eskultet@redhat.com> wrote:
On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
On Mon, 23 Jul 2018 at 18:39, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index dfd792f..b4a4d06 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list;
VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
va_end(list);
- VIR_FREE(arg); return;
no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg);
There could be an additional patch replacing the no_memory label with 'cleanup' with the obvious adjustments.
There are many functions in virfirewall.c where no_memory is used instead of cleanup. So I should change either all of them, or none of them.
Yep, so it should be all of them.
Also, according to libvirt hacking page, no_memory is a standard label [1]. Should we replace it then? [1]: https://libvirt.org/hacking.html#goto
On Tue, Jul 24, 2018 at 10:46:54PM +0530, Sukrit Bhatnagar wrote:
On Tue, 24 Jul 2018 at 11:54, Erik Skultety <eskultet@redhat.com> wrote:
On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
On Mon, 23 Jul 2018 at 18:39, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index dfd792f..b4a4d06 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list;
VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
va_end(list);
- VIR_FREE(arg); return;
no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg);
There could be an additional patch replacing the no_memory label with 'cleanup' with the obvious adjustments.
There are many functions in virfirewall.c where no_memory is used instead of cleanup. So I should change either all of them, or none of them.
Yep, so it should be all of them.
Also, according to libvirt hacking page, no_memory is a standard label [1]. Should we replace it then?
No, the hacking page is correct - 'no_memory' should be used where the label is specifically related to OOM handling. That is the case in this code snippet, as the label is explicitly setting "firewall->err = ENOMEM". Using "cleanup" here would be misleading, suggesting it was generic error handling.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Tue, Jul 24, 2018 at 10:46:54PM +0530, Sukrit Bhatnagar wrote:
On Tue, 24 Jul 2018 at 11:54, Erik Skultety <eskultet@redhat.com> wrote:
On Mon, Jul 23, 2018 at 11:18:07PM +0530, Sukrit Bhatnagar wrote:
On Mon, 23 Jul 2018 at 18:39, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:47PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index dfd792f..b4a4d06 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -511,7 +511,7 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall, virFirewallRulePtr rule, const char *fmt, ...) { - char *arg; + VIR_AUTOFREE(char *) arg = NULL; va_list list;
VIR_FIREWALL_RULE_RETURN_IF_ERROR(firewall, rule); @@ -525,13 +525,11 @@ void virFirewallRuleAddArgFormat(virFirewallPtr firewall,
va_end(list);
- VIR_FREE(arg); return;
no_memory: firewall->err = ENOMEM; va_end(list); - VIR_FREE(arg);
There could be an additional patch replacing the no_memory label with 'cleanup' with the obvious adjustments.
There are many functions in virfirewall.c where no_memory is used instead of cleanup. So I should change either all of them, or none of them.
Yep, so it should be all of them.
Also, according to libvirt hacking page, no_memory is a standard label [1]. Should we replace it then?
Fair enough, you can disregard my comment then. Erik
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virfirewall.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-) diff --git a/src/util/virfirewall.c b/src/util/virfirewall.c index b4a4d06..c786d76 100644 --- a/src/util/virfirewall.c +++ b/src/util/virfirewall.c @@ -119,14 +119,13 @@ virFirewallCheckUpdateLock(bool *lockflag, const char *const*args) { int status; /* Ignore failed commands without logging them */ - virCommandPtr cmd = virCommandNewArgs(args); + VIR_AUTOPTR(virCommand) cmd = virCommandNewArgs(args); if (virCommandRun(cmd, &status) < 0 || status) { VIR_INFO("locking not supported by %s", args[0]); } else { VIR_INFO("using locking for %s", args[0]); *lockflag = true; } - virCommandFree(cmd); } static void @@ -673,16 +672,15 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, { size_t i; const char *bin = virFirewallLayerCommandTypeToString(rule->layer); - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int status; - int ret = -1; VIR_AUTOFREE(char *) error = NULL; if (!bin) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unknown firewall layer %d"), rule->layer); - goto cleanup; + return -1; } cmd = virCommandNewArgList(bin, NULL); @@ -694,7 +692,7 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, virCommandSetErrorBuffer(cmd, &error); if (virCommandRun(cmd, &status) < 0) - goto cleanup; + return -1; if (status != 0) { if (ignoreErrors) { @@ -705,14 +703,11 @@ virFirewallApplyRuleDirect(virFirewallRulePtr rule, _("Failed to apply firewall rules %s: %s"), NULLSTR(args), NULLSTR(error)); VIR_FREE(*output); - goto cleanup; + return -1; } } - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return 0; } @@ -805,8 +800,7 @@ virFirewallApplyRule(virFirewallPtr firewall, { VIR_AUTOFREE(char *) output = NULL; VIR_AUTOFREE(char *) str = virFirewallRuleToString(rule); - char **lines = NULL; - int ret = -1; + VIR_AUTOPTR(virString) lines = NULL; VIR_INFO("Applying rule '%s'", NULLSTR(str)); if (rule->ignoreErrors) @@ -831,28 +825,25 @@ virFirewallApplyRule(virFirewallPtr firewall, if (rule->queryCB && output) { if (!(lines = virStringSplit(output, "\n", -1))) - goto cleanup; + return -1; VIR_DEBUG("Invoking query %p with '%s'", rule->queryCB, output); if (rule->queryCB(firewall, (const char *const *)lines, rule->queryOpaque) < 0) - goto cleanup; + return -1; if (firewall->err == ENOMEM) { virReportOOMError(); - goto cleanup; + return -1; } if (firewall->err) { virReportSystemError(firewall->err, "%s", _("Unable to create rule")); - goto cleanup; + return -1; } } - ret = 0; - cleanup: - virStringListFree(lines); - return ret; + return 0; } static int @@ -926,7 +917,7 @@ virFirewallApply(virFirewallPtr firewall) if (virFirewallApplyGroup(firewall, i) < 0) { VIR_DEBUG("Rolling back groups up to %zu for %p", i, firewall); size_t first = i; - virErrorPtr saved_error = virSaveLastError(); + VIR_AUTOPTR(virError) saved_error = virSaveLastError(); /* * Look at any inheritance markers to figure out @@ -947,7 +938,6 @@ virFirewallApply(virFirewallPtr firewall) } virSetError(saved_error); - virFreeError(saved_error); VIR_DEBUG("Done rolling back groups for %p", firewall); goto cleanup; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:48PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhook.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index facd74a..51f0eb5 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -122,8 +122,7 @@ static int virHooksFound = -1; static int virHookCheck(int no, const char *driver) { - char *path; - int ret; + VIR_AUTOFREE(char *) path = NULL; if (driver == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -139,18 +138,15 @@ virHookCheck(int no, const char *driver) } if (!virFileExists(path)) { - ret = 0; VIR_DEBUG("No hook script %s", path); + return 0; } else if (!virFileIsExecutable(path)) { - ret = 0; VIR_WARN("Non-executable hook script %s", path); + return 0; } else { - ret = 1; VIR_DEBUG("Found hook script %s", path); + return 1; } - - VIR_FREE(path); - return ret; } /* @@ -233,7 +229,7 @@ virHookCall(int driver, char **output) { int ret; - char *path; + VIR_AUTOFREE(char *) path = NULL; virCommandPtr cmd; const char *drvstr; const char *opstr; @@ -318,7 +314,5 @@ virHookCall(int driver, virCommandFree(cmd); - VIR_FREE(path); - return ret; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:49PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhook.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/src/util/virhook.c b/src/util/virhook.c index facd74a..51f0eb5 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -122,8 +122,7 @@ static int virHooksFound = -1; static int virHookCheck(int no, const char *driver) { - char *path; - int ret; + VIR_AUTOFREE(char *) path = NULL;
if (driver == NULL) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -139,18 +138,15 @@ virHookCheck(int no, const char *driver) }
if (!virFileExists(path)) { - ret = 0; VIR_DEBUG("No hook script %s", path); + return 0; } else if (!virFileIsExecutable(path)) { - ret = 0; VIR_WARN("Non-executable hook script %s", path); + return 0; } else { - ret = 1; VIR_DEBUG("Found hook script %s", path); + return 1;
^this should be reworked in the following way: if (!virFileExists) { VIR_DEBUG; return 0; } if (!virFileIsExecutable) { VIR_DEBUG; return 0; } VIR_DEBUG; return 1; Otherwise looks okay. Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhook.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index 51f0eb5..de80947 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -230,7 +230,7 @@ virHookCall(int driver, { int ret; VIR_AUTOFREE(char *) path = NULL; - virCommandPtr cmd; + VIR_AUTOPTR(virCommand) cmd = NULL; const char *drvstr; const char *opstr; const char *subopstr; @@ -312,7 +312,5 @@ virHookCall(int driver, virGetLastErrorMessage()); } - virCommandFree(cmd); - return ret; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:50PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When variables of type virPCIDevicePtr and virPCIEDeviceInfoPtr are declared using VIR_AUTOPTR, the functions virPCIDeviceFree and virPCIEDeviceInfoFree, respectively, will be run automatically on them when they go out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virpci.c | 1 - src/util/virpci.h | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 8d02366..a606462 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -39,7 +39,6 @@ #include "dirname.h" #include "virlog.h" -#include "viralloc.h" #include "vircommand.h" #include "virerror.h" #include "virfile.h" diff --git a/src/util/virpci.h b/src/util/virpci.h index 794b7e5..8fc8716 100644 --- a/src/util/virpci.h +++ b/src/util/virpci.h @@ -28,6 +28,7 @@ # include "virmdev.h" # include "virobject.h" # include "virutil.h" +# include "viralloc.h" typedef struct _virPCIDevice virPCIDevice; typedef virPCIDevice *virPCIDevicePtr; @@ -253,4 +254,7 @@ void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev); ssize_t virPCIGetMdevTypes(const char *sysfspath, virMediatedDeviceType ***types); +VIR_DEFINE_AUTOPTR_FUNC(virPCIDevice, virPCIDeviceFree) +VIR_DEFINE_AUTOPTR_FUNC(virPCIEDeviceInfo, virPCIEDeviceInfoFree) + #endif /* __VIR_PCI_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:51PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When variables of type virPCIDevicePtr and virPCIEDeviceInfoPtr are declared using VIR_AUTOPTR, the functions virPCIDeviceFree and virPCIEDeviceInfoFree, respectively, will be run automatically on them when they go out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virpci.c | 271 ++++++++++++++++++------------------------------------ 1 file changed, 87 insertions(+), 184 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index a606462..1f6ac0b 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -253,7 +253,7 @@ int virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) { int ret = -1; - char *drvlink = NULL; + VIR_AUTOFREE(char *) drvlink = NULL; *path = *name = NULL; /* drvlink = "/sys/bus/pci/dddd:bb:ss.ff/driver" */ @@ -285,7 +285,6 @@ virPCIDeviceGetDriverPathAndName(virPCIDevicePtr dev, char **path, char **name) ret = 0; cleanup: - VIR_FREE(drvlink); if (ret < 0) { VIR_FREE(*path); VIR_FREE(*name); @@ -375,32 +374,27 @@ virPCIDeviceRead32(virPCIDevicePtr dev, int cfgfd, unsigned int pos) static int virPCIDeviceReadClass(virPCIDevicePtr dev, uint16_t *device_class) { - char *path = NULL; - char *id_str = NULL; - int ret = -1; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) id_str = NULL; unsigned int value; if (!(path = virPCIFile(dev->name, "class"))) - return ret; + return -1; /* class string is '0xNNNNNN\n' ... i.e. 9 bytes */ if (virFileReadAll(path, 9, &id_str) < 0) - goto cleanup; + return -1; id_str[8] = '\0'; if (virStrToLong_ui(id_str, NULL, 16, &value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unusual value in %s/devices/%s/class: %s"), PCI_SYSFS, dev->name, id_str); - goto cleanup; + return -1; } *device_class = (value >> 8) & 0xFFFF; - ret = 0; - cleanup: - VIR_FREE(id_str); - VIR_FREE(path); - return ret; + return 0; } static int @@ -574,7 +568,7 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd) { uint32_t caps; uint8_t pos; - char *path; + VIR_AUTOFREE(char *) path = NULL; int found; /* The PCIe Function Level Reset capability allows @@ -614,7 +608,6 @@ virPCIDeviceDetectFunctionLevelReset(virPCIDevicePtr dev, int cfgfd) return -1; found = virFileExists(path); - VIR_FREE(path); if (found) { VIR_DEBUG("%s %s: buggy device didn't advertise FLR, but is a VF; forcing flr on", dev->id, dev->name); @@ -926,8 +919,8 @@ virPCIDeviceReset(virPCIDevicePtr dev, virPCIDeviceList *activeDevs, virPCIDeviceList *inactiveDevs) { - char *drvPath = NULL; - char *drvName = NULL; + VIR_AUTOFREE(char *) drvPath = NULL; + VIR_AUTOFREE(char *) drvName = NULL; int ret = -1; int fd = -1; int hdrType = -1; @@ -1000,8 +993,6 @@ virPCIDeviceReset(virPCIDevicePtr dev, } cleanup: - VIR_FREE(drvPath); - VIR_FREE(drvName); virPCIDeviceConfigClose(dev, fd); return ret; } @@ -1011,7 +1002,7 @@ static int virPCIProbeStubDriver(virPCIStubDriver driver) { const char *drvname = NULL; - char *drvpath = NULL; + VIR_AUTOFREE(char *) drvpath = NULL; bool probed = false; if (driver == VIR_PCI_STUB_DRIVER_NONE || @@ -1023,20 +1014,15 @@ virPCIProbeStubDriver(virPCIStubDriver driver) } recheck: - if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath)) { + if ((drvpath = virPCIDriverDir(drvname)) && virFileExists(drvpath)) /* driver already loaded, return */ - VIR_FREE(drvpath); return 0; - } - - VIR_FREE(drvpath); if (!probed) { - char *errbuf = NULL; + VIR_AUTOFREE(char *) errbuf = NULL; probed = true; if ((errbuf = virKModLoad(drvname, true))) { VIR_WARN("failed to load driver %s: %s", drvname, errbuf); - VIR_FREE(errbuf); goto cleanup; } @@ -1064,38 +1050,30 @@ virPCIProbeStubDriver(virPCIStubDriver driver) int virPCIDeviceUnbind(virPCIDevicePtr dev) { - char *path = NULL; - char *drvpath = NULL; - char *driver = NULL; - int ret = -1; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) drvpath = NULL; + VIR_AUTOFREE(char *) driver = NULL; if (virPCIDeviceGetDriverPathAndName(dev, &drvpath, &driver) < 0) - goto cleanup; + return -1; - if (!driver) { + if (!driver) /* The device is not bound to any driver */ - ret = 0; - goto cleanup; - } + return 0; if (!(path = virPCIFile(dev->name, "driver/unbind"))) - goto cleanup; + return -1; if (virFileExists(path)) { if (virFileWriteStr(path, dev->name, 0) < 0) { virReportSystemError(errno, _("Failed to unbind PCI device '%s' from %s"), dev->name, driver); - goto cleanup; + return -1; } } - ret = 0; - cleanup: - VIR_FREE(path); - VIR_FREE(drvpath); - VIR_FREE(driver); - return ret; + return 0; } @@ -1138,8 +1116,7 @@ static int virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev, const char *driverName) { - int ret = -1; - char *path; + VIR_AUTOFREE(char *) path = NULL; if (!(path = virPCIFile(dev->name, "driver_override"))) return -1; @@ -1149,26 +1126,22 @@ virPCIDeviceBindWithDriverOverride(virPCIDevicePtr dev, _("Failed to add driver '%s' to driver_override " " interface of PCI device '%s'"), driverName, dev->name); - goto cleanup; + return -1; } if (virPCIDeviceRebind(dev) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - VIR_FREE(path); - return ret; + return 0; } static int virPCIDeviceUnbindFromStubWithNewid(virPCIDevicePtr dev) { int result = -1; - char *drvdir = NULL; - char *path = NULL; - char *driver = NULL; + VIR_AUTOFREE(char *) drvdir = NULL; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) driver = NULL; /* If the device is currently bound to one of the "well known" * stub drivers, then unbind it, otherwise ignore it. @@ -1257,10 +1230,6 @@ virPCIDeviceUnbindFromStubWithNewid(virPCIDevicePtr dev) dev->remove_slot = false; dev->reprobe = false; - VIR_FREE(drvdir); - VIR_FREE(path); - VIR_FREE(driver); - return result; } @@ -1278,8 +1247,7 @@ virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev) static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) { - int ret; - char *path; + VIR_AUTOFREE(char *) path = NULL; /* * Prefer using the device's driver_override interface, falling back @@ -1289,12 +1257,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) return -1; if (virFileExists(path)) - ret = virPCIDeviceUnbindFromStubWithOverride(dev); + return virPCIDeviceUnbindFromStubWithOverride(dev); else - ret = virPCIDeviceUnbindFromStubWithNewid(dev); - - VIR_FREE(path); - return ret; + return virPCIDeviceUnbindFromStubWithNewid(dev); } static int @@ -1302,9 +1267,9 @@ virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) { int result = -1; bool reprobe = false; - char *stubDriverPath = NULL; - char *driverLink = NULL; - char *path = NULL; /* reused for different purposes */ + VIR_AUTOFREE(char *) stubDriverPath = NULL; + VIR_AUTOFREE(char *) driverLink = NULL; + VIR_AUTOFREE(char *) path = NULL; /* reused for different purposes */ const char *stubDriverName = NULL; virErrorPtr err = NULL; @@ -1436,10 +1401,6 @@ virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) } cleanup: - VIR_FREE(stubDriverPath); - VIR_FREE(driverLink); - VIR_FREE(path); - if (result < 0) virPCIDeviceUnbindFromStub(dev); @@ -1453,10 +1414,9 @@ virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) static int virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev) { - int ret = -1; const char *stubDriverName; - char *stubDriverPath = NULL; - char *driverLink = NULL; + VIR_AUTOFREE(char *) stubDriverPath = NULL; + VIR_AUTOFREE(char *) driverLink = NULL; /* Check the device is configured to use one of the known stub drivers */ if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) { @@ -1473,35 +1433,28 @@ virPCIDeviceBindToStubWithOverride(virPCIDevicePtr dev) if (!(stubDriverPath = virPCIDriverDir(stubDriverName)) || !(driverLink = virPCIFile(dev->name, "driver"))) - goto cleanup; + return -1; if (virFileExists(driverLink)) { if (virFileLinkPointsTo(driverLink, stubDriverPath)) { /* The device is already bound to the correct driver */ VIR_DEBUG("Device %s is already bound to %s", dev->name, stubDriverName); - ret = 0; - goto cleanup; + return 0; } } if (virPCIDeviceBindWithDriverOverride(dev, stubDriverName) < 0) - goto cleanup; + return -1; dev->unbind_from_stub = true; - ret = 0; - - cleanup: - VIR_FREE(stubDriverPath); - VIR_FREE(driverLink); - return ret; + return 0; } static int virPCIDeviceBindToStub(virPCIDevicePtr dev) { - int ret; - char *path; + VIR_AUTOFREE(char *) path = NULL; /* * Prefer using the device's driver_override interface, falling back @@ -1511,12 +1464,9 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) return -1; if (virFileExists(path)) - ret = virPCIDeviceBindToStubWithOverride(dev); + return virPCIDeviceBindToStubWithOverride(dev); else - ret = virPCIDeviceBindToStubWithNewid(dev); - - VIR_FREE(path); - return ret; + return virPCIDeviceBindToStubWithNewid(dev); } /* virPCIDeviceDetach: @@ -1700,19 +1650,15 @@ virPCIDeviceWaitForCleanup(virPCIDevicePtr dev, const char *matcher) static char * virPCIDeviceReadID(virPCIDevicePtr dev, const char *id_name) { - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; char *id_str; if (!(path = virPCIFile(dev->name, id_name))) return NULL; /* ID string is '0xNNNN\n' ... i.e. 7 bytes */ - if (virFileReadAll(path, 7, &id_str) < 0) { - VIR_FREE(path); + if (virFileReadAll(path, 7, &id_str) < 0) return NULL; - } - - VIR_FREE(path); /* Check for 0x suffix */ if (id_str[0] != '0' || id_str[1] != 'x') { @@ -1755,8 +1701,8 @@ virPCIDeviceNew(unsigned int domain, unsigned int function) { virPCIDevicePtr dev; - char *vendor = NULL; - char *product = NULL; + VIR_AUTOFREE(char *) vendor = NULL; + VIR_AUTOFREE(char *) product = NULL; if (VIR_ALLOC(dev) < 0) return NULL; @@ -1805,15 +1751,11 @@ virPCIDeviceNew(unsigned int domain, VIR_DEBUG("%s %s: initialized", dev->id, dev->name); - cleanup: - VIR_FREE(product); - VIR_FREE(vendor); return dev; error: virPCIDeviceFree(dev); - dev = NULL; - goto cleanup; + return NULL; } @@ -2129,8 +2071,7 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, virPCIDeviceFileActor actor, void *opaque) { - char *pcidir = NULL; - char *file = NULL; + VIR_AUTOFREE(char *) pcidir = NULL; DIR *dir = NULL; int ret = -1; struct dirent *ent; @@ -2145,6 +2086,7 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, goto cleanup; while ((direrr = virDirRead(dir, &ent, pcidir)) > 0) { + VIR_AUTOFREE(char *) file = NULL; /* Device assignment requires: * $PCIDIR/config, $PCIDIR/resource, $PCIDIR/resourceNNN, * $PCIDIR/rom, $PCIDIR/reset, $PCIDIR/vendor, $PCIDIR/device @@ -2159,8 +2101,6 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, goto cleanup; if ((actor)(dev, file, opaque) < 0) goto cleanup; - - VIR_FREE(file); } } if (direrr < 0) @@ -2170,8 +2110,6 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, cleanup: VIR_DIR_CLOSE(dir); - VIR_FREE(file); - VIR_FREE(pcidir); return ret; } @@ -2186,7 +2124,7 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, virPCIDeviceAddressActor actor, void *opaque) { - char *groupPath = NULL; + VIR_AUTOFREE(char *) groupPath = NULL; DIR *groupDir = NULL; int ret = -1; struct dirent *ent; @@ -2222,7 +2160,6 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, ret = 0; cleanup: - VIR_FREE(groupPath); VIR_DIR_CLOSE(groupDir); return ret; } @@ -2341,28 +2278,25 @@ virPCIDeviceAddressGetIOMMUGroupAddresses(virPCIDeviceAddressPtr devAddr, int virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) { - char *devName = NULL; - char *devPath = NULL; - char *groupPath = NULL; + VIR_AUTOFREE(char *) devName = NULL; + VIR_AUTOFREE(char *) devPath = NULL; + VIR_AUTOFREE(char *) groupPath = NULL; const char *groupNumStr; unsigned int groupNum; - int ret = -1; if (virAsprintf(&devName, "%.4x:%.2x:%.2x.%.1x", addr->domain, addr->bus, addr->slot, addr->function) < 0) - goto cleanup; + return -1; if (!(devPath = virPCIFile(devName, "iommu_group"))) - goto cleanup; - if (virFileIsLink(devPath) != 1) { - ret = -2; - goto cleanup; - } + return -1; + if (virFileIsLink(devPath) != 1) + return -2; if (virFileResolveLink(devPath, &groupPath) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to resolve device %s iommu_group symlink %s"), devName, devPath); - goto cleanup; + return -1; } groupNumStr = last_component(groupPath); @@ -2371,16 +2305,10 @@ virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) _("device %s iommu_group symlink %s has " "invalid group number %s"), devName, groupPath, groupNumStr); - ret = -1; - goto cleanup; + return -1; } - ret = groupNum; - cleanup: - VIR_FREE(devName); - VIR_FREE(devPath); - VIR_FREE(groupPath); - return ret; + return groupNum; } @@ -2390,30 +2318,28 @@ virPCIDeviceAddressGetIOMMUGroupNum(virPCIDeviceAddressPtr addr) char * virPCIDeviceGetIOMMUGroupDev(virPCIDevicePtr dev) { - char *devPath = NULL; - char *groupPath = NULL; + VIR_AUTOFREE(char *) devPath = NULL; + VIR_AUTOFREE(char *) groupPath = NULL; char *groupDev = NULL; if (!(devPath = virPCIFile(dev->name, "iommu_group"))) - goto cleanup; + return NULL; if (virFileIsLink(devPath) != 1) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid device %s iommu_group file %s is not a symlink"), dev->name, devPath); - goto cleanup; + return NULL; } if (virFileResolveLink(devPath, &groupPath) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to resolve device %s iommu_group symlink %s"), dev->name, devPath); - goto cleanup; + return NULL; } if (virAsprintf(&groupDev, "/dev/vfio/%s", last_component(groupPath)) < 0) - goto cleanup; - cleanup: - VIR_FREE(devPath); - VIR_FREE(groupPath); + return NULL; + return groupDev; } @@ -2614,7 +2540,7 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) { virPCIDeviceAddressPtr bdf = NULL; char *config_address = NULL; - char *device_path = NULL; + VIR_AUTOFREE(char *) device_path = NULL; if (!virFileExists(device_link)) { VIR_DEBUG("'%s' does not exist", device_link); @@ -2631,19 +2557,16 @@ virPCIGetDeviceAddressFromSysfsLink(const char *device_link) config_address = last_component(device_path); if (VIR_ALLOC(bdf) < 0) - goto out; + return NULL; if (virPCIDeviceAddressParse(config_address, bdf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to parse PCI config address '%s'"), config_address); VIR_FREE(bdf); - goto out; + return NULL; } - out: - VIR_FREE(device_path); - return bdf; } @@ -2665,7 +2588,7 @@ int virPCIGetPhysicalFunction(const char *vf_sysfs_path, virPCIDeviceAddressPtr *pf) { - char *device_link = NULL; + VIR_AUTOFREE(char *) device_link = NULL; *pf = NULL; @@ -2678,7 +2601,6 @@ virPCIGetPhysicalFunction(const char *vf_sysfs_path, VIR_DEBUG("PF for VF device '%s': %.4x:%.2x:%.2x.%.1x", vf_sysfs_path, (*pf)->domain, (*pf)->bus, (*pf)->slot, (*pf)->function); } - VIR_FREE(device_link); return 0; } @@ -2695,9 +2617,9 @@ virPCIGetVirtualFunctions(const char *sysfs_path, { int ret = -1; size_t i; - char *device_link = NULL; + VIR_AUTOFREE(char *) totalvfs_file = NULL; + VIR_AUTOFREE(char *) totalvfs_str = NULL; virPCIDeviceAddressPtr config_addr = NULL; - char *totalvfs_file = NULL, *totalvfs_str = NULL; *virtual_functions = NULL; *num_virtual_functions = 0; @@ -2719,6 +2641,7 @@ virPCIGetVirtualFunctions(const char *sysfs_path, } do { + VIR_AUTOFREE(char *) device_link = NULL; /* look for virtfn%d links until one isn't found */ if (virAsprintf(&device_link, "%s/virtfn%zu", sysfs_path, *num_virtual_functions) < 0) goto error; @@ -2736,18 +2659,13 @@ virPCIGetVirtualFunctions(const char *sysfs_path, if (VIR_APPEND_ELEMENT(*virtual_functions, *num_virtual_functions, config_addr) < 0) goto error; - VIR_FREE(device_link); - } while (1); VIR_DEBUG("Found %zu virtual functions for %s", *num_virtual_functions, sysfs_path); ret = 0; cleanup: - VIR_FREE(device_link); VIR_FREE(config_addr); - VIR_FREE(totalvfs_file); - VIR_FREE(totalvfs_str); return ret; error: @@ -2765,18 +2683,13 @@ virPCIGetVirtualFunctions(const char *sysfs_path, int virPCIIsVirtualFunction(const char *vf_sysfs_device_link) { - char *vf_sysfs_physfn_link = NULL; - int ret = -1; + VIR_AUTOFREE(char *) vf_sysfs_physfn_link = NULL; if (virAsprintf(&vf_sysfs_physfn_link, "%s/physfn", vf_sysfs_device_link) < 0) - return ret; + return -1; - ret = virFileExists(vf_sysfs_physfn_link); - - VIR_FREE(vf_sysfs_physfn_link); - - return ret; + return virFileExists(vf_sysfs_physfn_link); } /* @@ -2868,12 +2781,12 @@ virPCIGetNetName(const char *device_link_sysfs_path, char *physPortID, char **netname) { - char *pcidev_sysfs_net_path = NULL; + VIR_AUTOFREE(char *) pcidev_sysfs_net_path = NULL; + VIR_AUTOFREE(char *) firstEntryName = NULL; + VIR_AUTOFREE(char *) thisPhysPortID = NULL; int ret = -1; DIR *dir = NULL; struct dirent *entry = NULL; - char *firstEntryName = NULL; - char *thisPhysPortID = NULL; size_t i = 0; if (virBuildPath(&pcidev_sysfs_net_path, device_link_sysfs_path, @@ -2946,9 +2859,6 @@ virPCIGetNetName(const char *device_link_sysfs_path, } cleanup: VIR_DIR_CLOSE(dir); - VIR_FREE(pcidev_sysfs_net_path); - VIR_FREE(thisPhysPortID); - VIR_FREE(firstEntryName); return ret; } @@ -2959,9 +2869,9 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, int *vf_index) { virPCIDeviceAddressPtr pf_config_address = NULL; - char *pf_sysfs_device_path = NULL; - char *vfname = NULL; - char *vfPhysPortID = NULL; + VIR_AUTOFREE(char *) pf_sysfs_device_path = NULL; + VIR_AUTOFREE(char *) vfname = NULL; + VIR_AUTOFREE(char *) vfPhysPortID = NULL; int ret = -1; if (virPCIGetPhysicalFunction(vf_sysfs_device_path, &pf_config_address) < 0) @@ -3016,9 +2926,6 @@ virPCIGetVirtualFunctionInfo(const char *vf_sysfs_device_path, ret = 0; cleanup: VIR_FREE(pf_config_address); - VIR_FREE(pf_sysfs_device_path); - VIR_FREE(vfname); - VIR_FREE(vfPhysPortID); return ret; } @@ -3032,8 +2939,7 @@ virPCIGetMdevTypes(const char *sysfspath, int dirret = -1; DIR *dir = NULL; struct dirent *entry; - char *types_path = NULL; - char *tmppath = NULL; + VIR_AUTOFREE(char *) types_path = NULL; virMediatedDeviceTypePtr mdev_type = NULL; virMediatedDeviceTypePtr *mdev_types = NULL; size_t ntypes = 0; @@ -3051,6 +2957,7 @@ virPCIGetMdevTypes(const char *sysfspath, } while ((dirret = virDirRead(dir, &entry, types_path)) > 0) { + VIR_AUTOFREE(char *) tmppath = NULL; /* append the type id to the path and read the attributes from there */ if (virAsprintf(&tmppath, "%s/%s", types_path, entry->d_name) < 0) goto cleanup; @@ -3060,8 +2967,6 @@ virPCIGetMdevTypes(const char *sysfspath, if (VIR_APPEND_ELEMENT(mdev_types, ntypes, mdev_type) < 0) goto cleanup; - - VIR_FREE(tmppath); } if (dirret < 0) @@ -3075,8 +2980,6 @@ virPCIGetMdevTypes(const char *sysfspath, for (i = 0; i < ntypes; i++) virMediatedDeviceTypeFree(mdev_types[i]); VIR_FREE(mdev_types); - VIR_FREE(types_path); - VIR_FREE(tmppath); VIR_DIR_CLOSE(dir); return ret; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:52PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
...
@@ -1278,8 +1247,7 @@ virPCIDeviceUnbindFromStubWithOverride(virPCIDevicePtr dev) static int virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) { - int ret; - char *path; + VIR_AUTOFREE(char *) path = NULL;
/* * Prefer using the device's driver_override interface, falling back @@ -1289,12 +1257,9 @@ virPCIDeviceUnbindFromStub(virPCIDevicePtr dev) return -1;
if (virFileExists(path)) - ret = virPCIDeviceUnbindFromStubWithOverride(dev); + return virPCIDeviceUnbindFromStubWithOverride(dev); else
Drop the 'else' clause, it's not needed anymore. ...
static int virPCIDeviceBindToStub(virPCIDevicePtr dev) { - int ret; - char *path; + VIR_AUTOFREE(char *) path = NULL;
/* * Prefer using the device's driver_override interface, falling back @@ -1511,12 +1464,9 @@ virPCIDeviceBindToStub(virPCIDevicePtr dev) return -1;
if (virFileExists(path)) - ret = virPCIDeviceBindToStubWithOverride(dev); + return virPCIDeviceBindToStubWithOverride(dev); else ... here too..
...
@@ -1755,8 +1701,8 @@ virPCIDeviceNew(unsigned int domain, unsigned int function) { virPCIDevicePtr dev;
virPCIDevicePtr ret = NULL; VIR_AUTOPTR(virPCIDevicePtr) tmp = NULL;
- char *vendor = NULL; - char *product = NULL; + VIR_AUTOFREE(char *) vendor = NULL; + VIR_AUTOFREE(char *) product = NULL;
if (VIR_ALLOC(dev) < 0) return NULL; @@ -1805,15 +1751,11 @@ virPCIDeviceNew(unsigned int domain,
VIR_DEBUG("%s %s: initialized", dev->id, dev->name);
VIR_STEAL_PTR(ret, dev);
- cleanup:
Preserve the 'cleanup' label...
- VIR_FREE(product); - VIR_FREE(vendor); return dev;
error:
Drop 'error' label
virPCIDeviceFree(dev);
^this can be dropped too...
- dev = NULL; - goto cleanup; + return NULL;
^return ret; Erik
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virpci.c | 51 ++++++++++++++++++--------------------------------- 1 file changed, 18 insertions(+), 33 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 1f6ac0b..7b0964c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -490,8 +490,6 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, ret = 1; break; } - - virPCIDeviceFree(check); } VIR_DIR_CLOSE(dir); return ret; @@ -781,7 +779,8 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev, int cfgfd, virPCIDeviceList *inactiveDevs) { - virPCIDevicePtr parent, conflict; + VIR_AUTOPTR(virPCIDevice) parent = NULL; + VIR_AUTOPTR(virPCIDevice) conflict = NULL; uint8_t config_space[PCI_CONF_LEN]; uint16_t ctl; int ret = -1; @@ -795,7 +794,6 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev, virReportError(VIR_ERR_INTERNAL_ERROR, _("Active %s devices on bus with %s, not doing bus reset"), conflict->name, dev->name); - virPCIDeviceFree(conflict); return -1; } @@ -848,7 +846,6 @@ virPCIDeviceTrySecondaryBusReset(virPCIDevicePtr dev, out: virPCIDeviceConfigClose(parent, parentfd); - virPCIDeviceFree(parent); return ret; } @@ -1270,8 +1267,8 @@ virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) VIR_AUTOFREE(char *) stubDriverPath = NULL; VIR_AUTOFREE(char *) driverLink = NULL; VIR_AUTOFREE(char *) path = NULL; /* reused for different purposes */ + VIR_AUTOPTR(virError) err = NULL; const char *stubDriverName = NULL; - virErrorPtr err = NULL; /* Check the device is configured to use one of the known stub drivers */ if (dev->stubDriver == VIR_PCI_STUB_DRIVER_NONE) { @@ -1406,7 +1403,6 @@ virPCIDeviceBindToStubWithNewid(virPCIDevicePtr dev) if (err) virSetError(err); - virFreeError(err); return result; } @@ -1679,19 +1675,16 @@ virPCIGetAddrString(unsigned int domain, unsigned int function, char **pciConfigAddr) { - virPCIDevicePtr dev = NULL; - int ret = -1; + VIR_AUTOPTR(virPCIDevice) dev = NULL; dev = virPCIDeviceNew(domain, bus, slot, function); if (dev != NULL) { if (VIR_STRDUP(*pciConfigAddr, dev->name) < 0) - goto cleanup; - ret = 0; + return -1; + return 0; } - cleanup: - virPCIDeviceFree(dev); - return ret; + return -1; } virPCIDevicePtr @@ -1962,10 +1955,10 @@ virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev) if (!copy) return -1; - if (virPCIDeviceListAdd(list, copy) < 0) { - virPCIDeviceFree(copy); + if (virPCIDeviceListAdd(list, copy) < 0) return -1; - } + + copy = NULL; return 0; } @@ -2013,8 +2006,7 @@ void virPCIDeviceListDel(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - virPCIDevicePtr ret = virPCIDeviceListSteal(list, dev); - virPCIDeviceFree(ret); + VIR_AUTOPTR(virPCIDevice) ret = virPCIDeviceListSteal(list, dev); } int @@ -2168,22 +2160,18 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, static int virPCIDeviceGetIOMMUGroupAddOne(virPCIDeviceAddressPtr newDevAddr, void *opaque) { - int ret = -1; virPCIDeviceListPtr groupList = opaque; - virPCIDevicePtr newDev; + VIR_AUTOPTR(virPCIDevice) newDev = NULL; if (!(newDev = virPCIDeviceNew(newDevAddr->domain, newDevAddr->bus, newDevAddr->slot, newDevAddr->function))) - goto cleanup; + return -1; if (virPCIDeviceListAdd(groupList, newDev) < 0) - goto cleanup; + return -1; newDev = NULL; /* it's now on the list */ - ret = 0; - cleanup: - virPCIDeviceFree(newDev); - return ret; + return 0; } @@ -2395,7 +2383,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev) static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) { - virPCIDevicePtr parent; + VIR_AUTOPTR(virPCIDevice) parent = NULL; if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; @@ -2419,14 +2407,13 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) * parent can be found */ do { - virPCIDevicePtr tmp; + VIR_AUTOPTR(virPCIDevice) tmp = NULL; int acs; int ret; acs = virPCIDeviceDownstreamLacksACS(parent); if (acs) { - virPCIDeviceFree(parent); if (acs < 0) return -1; else @@ -2435,7 +2422,6 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) tmp = parent; ret = virPCIDeviceGetParent(parent, &parent); - virPCIDeviceFree(tmp); if (ret < 0) return -1; } while (parent); @@ -2940,7 +2926,7 @@ virPCIGetMdevTypes(const char *sysfspath, DIR *dir = NULL; struct dirent *entry; VIR_AUTOFREE(char *) types_path = NULL; - virMediatedDeviceTypePtr mdev_type = NULL; + VIR_AUTOPTR(virMediatedDeviceType) mdev_type = NULL; virMediatedDeviceTypePtr *mdev_types = NULL; size_t ntypes = 0; size_t i; @@ -2976,7 +2962,6 @@ virPCIGetMdevTypes(const char *sysfspath, ret = ntypes; ntypes = 0; cleanup: - virMediatedDeviceTypeFree(mdev_type); for (i = 0; i < ntypes; i++) virMediatedDeviceTypeFree(mdev_types[i]); VIR_FREE(mdev_types); -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:53PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virpci.c | 51 ++++++++++++++++++--------------------------------- 1 file changed, 18 insertions(+), 33 deletions(-)
diff --git a/src/util/virpci.c b/src/util/virpci.c index 1f6ac0b..7b0964c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -490,8 +490,6 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, ret = 1; break; } - - virPCIDeviceFree(check); } VIR_DIR_CLOSE(dir); return ret;
...
@@ -1679,19 +1675,16 @@ virPCIGetAddrString(unsigned int domain, unsigned int function, char **pciConfigAddr) { - virPCIDevicePtr dev = NULL; - int ret = -1; + VIR_AUTOPTR(virPCIDevice) dev = NULL;
dev = virPCIDeviceNew(domain, bus, slot, function); if (dev != NULL) { if (VIR_STRDUP(*pciConfigAddr, dev->name) < 0) - goto cleanup; - ret = 0; + return -1; + return 0;
if (!dev || VIR_STRDUP(*pciConfigAddr, dev->name) < 0)) return -1;
}
- cleanup: - virPCIDeviceFree(dev); - return ret; + return -1;
^return 0;
}
virPCIDevicePtr @@ -1962,10 +1955,10 @@ virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev)
if (!copy) return -1; - if (virPCIDeviceListAdd(list, copy) < 0) { - virPCIDeviceFree(copy); + if (virPCIDeviceListAdd(list, copy) < 0) return -1; - } + + copy = NULL;
You'll first need to define copy as VIR_AUTOPTR, if you want to use ^this assignment, otherwise it's a NOP. There may be some leftovers from the previous patch that will need to go into this one, otherwise looks fine. Erik
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virNetDevVlanPtr is declared using VIR_AUTOPTR, the function virNetDevVlanFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virnetdevvlan.c | 1 - src/util/virnetdevvlan.h | 4 ++++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virnetdevvlan.c b/src/util/virnetdevvlan.c index 4c8bce5..0afc47b 100644 --- a/src/util/virnetdevvlan.c +++ b/src/util/virnetdevvlan.c @@ -24,7 +24,6 @@ #include "internal.h" #include "virerror.h" #include "virnetdevvlan.h" -#include "viralloc.h" #define VIR_FROM_THIS VIR_FROM_NONE diff --git a/src/util/virnetdevvlan.h b/src/util/virnetdevvlan.h index 7f63626..be85f59 100644 --- a/src/util/virnetdevvlan.h +++ b/src/util/virnetdevvlan.h @@ -23,6 +23,8 @@ # include <virutil.h> +# include "viralloc.h" + typedef enum { VIR_NATIVE_VLAN_MODE_DEFAULT = 0, VIR_NATIVE_VLAN_MODE_TAGGED, @@ -48,4 +50,6 @@ void virNetDevVlanFree(virNetDevVlanPtr vlan); int virNetDevVlanEqual(const virNetDevVlan *a, const virNetDevVlan *b); int virNetDevVlanCopy(virNetDevVlanPtr dst, const virNetDevVlan *src); +VIR_DEFINE_AUTOPTR_FUNC(virNetDevVlan, virNetDevVlanFree) + #endif /* __VIR_NETDEV_VLAN_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:54PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When a variable of type virNetDevVlanPtr is declared using VIR_AUTOPTR, the function virNetDevVlanFree will be run automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
This isn't used until patch 32, so it should be moved before patch 31 handling hostdevs. Reviewed-by: Erik Skultety <eskultet@redhat.com>
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virUSBDevicePtr is declared using VIR_AUTOPTR, the function virUSBDeviceFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virusb.c | 1 - src/util/virusb.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virusb.c b/src/util/virusb.c index 2fe1bfc..0a8dd0d 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -35,7 +35,6 @@ #include "virusb.h" #include "virlog.h" -#include "viralloc.h" #include "virutil.h" #include "virerror.h" #include "virfile.h" diff --git a/src/util/virusb.h b/src/util/virusb.h index 716e8c6..5cb9faa 100644 --- a/src/util/virusb.h +++ b/src/util/virusb.h @@ -27,6 +27,7 @@ # include "internal.h" # include "virobject.h" +# include "viralloc.h" # define USB_DEVFS "/dev/bus/usb/" @@ -99,4 +100,6 @@ void virUSBDeviceListDel(virUSBDeviceListPtr list, virUSBDevicePtr virUSBDeviceListFind(virUSBDeviceListPtr list, virUSBDevicePtr dev); +VIR_DEFINE_AUTOPTR_FUNC(virUSBDevice, virUSBDeviceFree) + #endif /* __VIR_USB_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:55PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When a variable of type virUSBDevicePtr is declared using VIR_AUTOPTR, the function virUSBDeviceFree will be run automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virusb.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/util/virusb.c b/src/util/virusb.c index 0a8dd0d..c14683f 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -90,29 +90,25 @@ VIR_ONCE_GLOBAL_INIT(virUSB) static int virUSBSysReadFile(const char *f_name, const char *d_name, int base, unsigned int *value) { - int ret = -1, tmp; - char *buf = NULL; - char *filename = NULL; + int tmp; + VIR_AUTOFREE(char *) buf = NULL; + VIR_AUTOFREE(char *) filename = NULL; char *ignore = NULL; tmp = virAsprintf(&filename, USB_SYSFS "/devices/%s/%s", d_name, f_name); if (tmp < 0) - goto cleanup; + return -1; if (virFileReadAll(filename, 1024, &buf) < 0) - goto cleanup; + return -1; if (virStrToLong_ui(buf, &ignore, base, value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Could not parse usb file %s"), filename); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(filename); - VIR_FREE(buf); - return ret; + return 0; } static virUSBDeviceListPtr -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:56PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virusb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/virusb.c b/src/util/virusb.c index c14683f..cfeac51 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -508,8 +508,7 @@ void virUSBDeviceListDel(virUSBDeviceListPtr list, virUSBDevicePtr dev) { - virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev); - virUSBDeviceFree(ret); + VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev); } virUSBDevicePtr -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virusb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virusb.c b/src/util/virusb.c index c14683f..cfeac51 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -508,8 +508,7 @@ void virUSBDeviceListDel(virUSBDeviceListPtr list, virUSBDevicePtr dev) { - virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev); - virUSBDeviceFree(ret); + VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev); }
Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a double pointer to @dev instead of a single pointer. A bit more background info - the current issue is that virUSBDeviceListAdd calls our VIR_APPEND_ELEMENT helper which does clear the original pointer which we could utilize here, but not while passing a single pointer. Not a deal breaker, though, it's just a nice to have, since you're already working in this area, because I don't suppose we'd make such a change any time soon after your assignment is over. (regardless) Reviewed-by: Erik Skultety <eskultet@redhat.com>
On Wed, 25 Jul 2018 at 15:03, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virusb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virusb.c b/src/util/virusb.c index c14683f..cfeac51 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -508,8 +508,7 @@ void virUSBDeviceListDel(virUSBDeviceListPtr list, virUSBDevicePtr dev) { - virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev); - virUSBDeviceFree(ret); + VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev); }
Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a double pointer to @dev instead of a single pointer. A bit more background info - the current issue is that virUSBDeviceListAdd calls our VIR_APPEND_ELEMENT helper which does clear the original pointer which we could utilize here, but not while passing a single pointer. Not a deal breaker, though, it's just a nice to have, since you're already working in this area, because I don't suppose we'd make such a change any time soon after your assignment is over.
(regardless) Reviewed-by: Erik Skultety <eskultet@redhat.com>
There are many such functions: virMediatedDeviceListAdd virSCSIDeviceListAdd virPCIDeviceListAdd Making those changes would take a while and it is not directly related to our cleanup. So, I'll do the necessary after the two cleanup macros are used in all files. Is that ok?
On Thu, 26 Jul 2018 at 00:32, Sukrit Bhatnagar <skrtbhtngr@gmail.com> wrote:
On Wed, 25 Jul 2018 at 15:03, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virusb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virusb.c b/src/util/virusb.c index c14683f..cfeac51 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -508,8 +508,7 @@ void virUSBDeviceListDel(virUSBDeviceListPtr list, virUSBDevicePtr dev) { - virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev); - virUSBDeviceFree(ret); + VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev); }
Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a double pointer to @dev instead of a single pointer. A bit more background info - the current issue is that virUSBDeviceListAdd calls our VIR_APPEND_ELEMENT helper which does clear the original pointer which we could utilize here, but not while passing a single pointer. Not a deal breaker, though, it's just a nice to have, since you're already working in this area, because I don't suppose we'd make such a change any time soon after your assignment is over.
(regardless) Reviewed-by: Erik Skultety <eskultet@redhat.com>
There are many such functions: virMediatedDeviceListAdd virSCSIDeviceListAdd virPCIDeviceListAdd
Making those changes would take a while and it is not directly related to our cleanup. So, I'll do the necessary after the two cleanup macros are used in all files. Is that ok?
For now, I am changing virUSBDeviceListAdd and the related occurrences.
On Thu, Jul 26, 2018 at 12:32:41AM +0530, Sukrit Bhatnagar wrote:
On Wed, 25 Jul 2018 at 15:03, Erik Skultety <eskultet@redhat.com> wrote:
On Sat, Jul 21, 2018 at 05:36:57PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virusb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/util/virusb.c b/src/util/virusb.c index c14683f..cfeac51 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -508,8 +508,7 @@ void virUSBDeviceListDel(virUSBDeviceListPtr list, virUSBDevicePtr dev) { - virUSBDevicePtr ret = virUSBDeviceListSteal(list, dev); - virUSBDeviceFree(ret); + VIR_AUTOPTR(virUSBDevice) ret = virUSBDeviceListSteal(list, dev); }
Technically, there's also a virUSBDevicePtr instance in virUSBDeviceSearch that could be converted to VIR_AUTOPTR, but virUSBDeviceListAdd would have to take a double pointer to @dev instead of a single pointer. A bit more background info - the current issue is that virUSBDeviceListAdd calls our VIR_APPEND_ELEMENT helper which does clear the original pointer which we could utilize here, but not while passing a single pointer. Not a deal breaker, though, it's just a nice to have, since you're already working in this area, because I don't suppose we'd make such a change any time soon after your assignment is over.
(regardless) Reviewed-by: Erik Skultety <eskultet@redhat.com>
There are many such functions: virMediatedDeviceListAdd virSCSIDeviceListAdd virPCIDeviceListAdd
Making those changes would take a while and it is not directly related to our cleanup. So, I'll do the necessary after the two cleanup macros are used in all files. Is that ok?
Yeah, as I said, it's just "nice-to-have", I understand we are short of time, so don't bother for now. Erik
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When variables of type virSCSIDevicePtr and virUsedByInfoPtr are declared using VIR_AUTOPTR, the functions virSCSIDeviceFree and virSCSIDeviceUsedByInfoFree, respectively, will be run automatically on them when they go out of scope. This commit also adds an intermediate typedef for virCgroup type for use with the cleanup macros by renaming the struct in src/util/vircgrouppriv.h from virCgroup to _virCgroup. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virscsi.c | 5 +++-- src/util/virscsi.h | 3 +++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/util/virscsi.c b/src/util/virscsi.c index b51103a..33292f6 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -37,7 +37,6 @@ #include "virlog.h" #include "virscsi.h" -#include "viralloc.h" #include "virfile.h" #include "virutil.h" #include "virstring.h" @@ -54,7 +53,8 @@ struct _virUsedByInfo { char *drvname; /* which driver */ char *domname; /* which domain */ }; -typedef struct _virUsedByInfo *virUsedByInfoPtr; +typedef struct _virUsedByInfo virUsedByInfo; +typedef virUsedByInfo *virUsedByInfoPtr; struct _virSCSIDevice { unsigned int adapter; @@ -264,6 +264,7 @@ virSCSIDeviceUsedByInfoFree(virUsedByInfoPtr used_by) VIR_FREE(used_by->domname); VIR_FREE(used_by); } +VIR_DEFINE_AUTOPTR_FUNC(virUsedByInfo, virSCSIDeviceUsedByInfoFree) void virSCSIDeviceFree(virSCSIDevicePtr dev) diff --git a/src/util/virscsi.h b/src/util/virscsi.h index 9f8b3ec..b96d862 100644 --- a/src/util/virscsi.h +++ b/src/util/virscsi.h @@ -26,6 +26,7 @@ # include "internal.h" # include "virobject.h" +# include "viralloc.h" typedef struct _virSCSIDevice virSCSIDevice; typedef virSCSIDevice *virSCSIDevicePtr; @@ -95,4 +96,6 @@ void virSCSIDeviceListDel(virSCSIDeviceListPtr list, virSCSIDevicePtr virSCSIDeviceListFind(virSCSIDeviceListPtr list, virSCSIDevicePtr dev); +VIR_DEFINE_AUTOPTR_FUNC(virSCSIDevice, virSCSIDeviceFree) + #endif /* __VIR_SCSI_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:58PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When variables of type virSCSIDevicePtr and virUsedByInfoPtr are declared using VIR_AUTOPTR, the functions virSCSIDeviceFree and virSCSIDeviceUsedByInfoFree, respectively, will be run automatically on them when they go out of scope.
This commit also adds an intermediate typedef for virCgroup type for use with the cleanup macros by renaming the struct in src/util/vircgrouppriv.h from virCgroup to _virCgroup.
You need to tweak ^this copy-paste to match the module ;). Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virscsi.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 33292f6..ba0a688 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -115,7 +115,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, { DIR *dir = NULL; struct dirent *entry; - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; char *sg = NULL; unsigned int adapter_id; const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; @@ -139,7 +139,6 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, cleanup: VIR_DIR_CLOSE(dir); - VIR_FREE(path); return sg; } @@ -155,7 +154,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, { DIR *dir = NULL; struct dirent *entry; - char *path = NULL; + VIR_AUTOFREE(char *) path = NULL; char *name = NULL; unsigned int adapter_id; const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; @@ -178,7 +177,6 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, cleanup: VIR_DIR_CLOSE(dir); - VIR_FREE(path); return name; } @@ -192,11 +190,11 @@ virSCSIDeviceNew(const char *sysfs_prefix, bool shareable) { virSCSIDevicePtr dev, ret = NULL; - char *sg = NULL; - char *vendor_path = NULL; - char *model_path = NULL; - char *vendor = NULL; - char *model = NULL; + VIR_AUTOFREE(char *) sg = NULL; + VIR_AUTOFREE(char *) vendor_path = NULL; + VIR_AUTOFREE(char *) model_path = NULL; + VIR_AUTOFREE(char *) vendor = NULL; + VIR_AUTOFREE(char *) model = NULL; const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; if (VIR_ALLOC(dev) < 0) @@ -247,11 +245,6 @@ virSCSIDeviceNew(const char *sysfs_prefix, ret = dev; cleanup: - VIR_FREE(sg); - VIR_FREE(vendor); - VIR_FREE(model); - VIR_FREE(vendor_path); - VIR_FREE(model_path); if (!ret) virSCSIDeviceFree(dev); return ret; -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:36:59PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virscsi.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/src/util/virscsi.c b/src/util/virscsi.c index ba0a688..abe699b 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -189,7 +189,8 @@ virSCSIDeviceNew(const char *sysfs_prefix, bool readonly, bool shareable) { - virSCSIDevicePtr dev, ret = NULL; + VIR_AUTOPTR(virSCSIDevice) dev = NULL; + virSCSIDevicePtr ret = NULL; VIR_AUTOFREE(char *) sg = NULL; VIR_AUTOFREE(char *) vendor_path = NULL; VIR_AUTOFREE(char *) model_path = NULL; @@ -207,46 +208,44 @@ virSCSIDeviceNew(const char *sysfs_prefix, dev->shareable = shareable; if (!(sg = virSCSIDeviceGetSgName(prefix, adapter, bus, target, unit))) - goto cleanup; + return NULL; if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) - goto cleanup; + return NULL; if (virAsprintf(&dev->name, "%d:%u:%u:%llu", dev->adapter, dev->bus, dev->target, dev->unit) < 0 || virAsprintf(&dev->sg_path, "%s/%s", sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0) - goto cleanup; + return NULL; if (!virFileExists(dev->sg_path)) { virReportSystemError(errno, _("SCSI device '%s': could not access %s"), dev->name, dev->sg_path); - goto cleanup; + return NULL; } if (virAsprintf(&vendor_path, "%s/%s/vendor", prefix, dev->name) < 0 || virAsprintf(&model_path, "%s/%s/model", prefix, dev->name) < 0) - goto cleanup; + return NULL; if (virFileReadAll(vendor_path, 1024, &vendor) < 0) - goto cleanup; + return NULL; if (virFileReadAll(model_path, 1024, &model) < 0) - goto cleanup; + return NULL; virTrimSpaces(vendor, NULL); virTrimSpaces(model, NULL); if (virAsprintf(&dev->id, "%s:%s", vendor, model) < 0) - goto cleanup; + return NULL; ret = dev; - cleanup: - if (!ret) - virSCSIDeviceFree(dev); + dev = NULL; return ret; } @@ -281,21 +280,17 @@ virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *drvname, const char *domname) { - virUsedByInfoPtr copy; + VIR_AUTOPTR(virUsedByInfo) copy = NULL; if (VIR_ALLOC(copy) < 0) return -1; if (VIR_STRDUP(copy->drvname, drvname) < 0 || VIR_STRDUP(copy->domname, domname) < 0) - goto cleanup; + return -1; if (VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy) < 0) - goto cleanup; + return -1; return 0; - - cleanup: - virSCSIDeviceUsedByInfoFree(copy); - return -1; } bool @@ -442,7 +437,6 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list, const char *drvname, const char *domname) { - virSCSIDevicePtr tmp = NULL; size_t i; for (i = 0; i < dev->n_used_by; i++) { @@ -452,8 +446,8 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list, virSCSIDeviceUsedByInfoFree(dev->used_by[i]); VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); } else { - tmp = virSCSIDeviceListSteal(list, dev); - virSCSIDeviceFree(tmp); + VIR_AUTOPTR(virSCSIDevice) tmp = + virSCSIDeviceListSteal(list, dev); } break; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:00PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virscsi.c | 38 ++++++++++++++++---------------------- 1 file changed, 16 insertions(+), 22 deletions(-)
diff --git a/src/util/virscsi.c b/src/util/virscsi.c index ba0a688..abe699b 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -189,7 +189,8 @@ virSCSIDeviceNew(const char *sysfs_prefix, bool readonly, bool shareable) { - virSCSIDevicePtr dev, ret = NULL; + VIR_AUTOPTR(virSCSIDevice) dev = NULL; + virSCSIDevicePtr ret = NULL; VIR_AUTOFREE(char *) sg = NULL; VIR_AUTOFREE(char *) vendor_path = NULL; VIR_AUTOFREE(char *) model_path = NULL; @@ -207,46 +208,44 @@ virSCSIDeviceNew(const char *sysfs_prefix, dev->shareable = shareable;
if (!(sg = virSCSIDeviceGetSgName(prefix, adapter, bus, target, unit))) - goto cleanup; + return NULL;
if (virSCSIDeviceGetAdapterId(adapter, &dev->adapter) < 0) - goto cleanup; + return NULL;
if (virAsprintf(&dev->name, "%d:%u:%u:%llu", dev->adapter, dev->bus, dev->target, dev->unit) < 0 || virAsprintf(&dev->sg_path, "%s/%s", sysfs_prefix ? sysfs_prefix : "/dev", sg) < 0) - goto cleanup; + return NULL;
if (!virFileExists(dev->sg_path)) { virReportSystemError(errno, _("SCSI device '%s': could not access %s"), dev->name, dev->sg_path); - goto cleanup; + return NULL; }
if (virAsprintf(&vendor_path, "%s/%s/vendor", prefix, dev->name) < 0 || virAsprintf(&model_path, "%s/%s/model", prefix, dev->name) < 0) - goto cleanup; + return NULL;
if (virFileReadAll(vendor_path, 1024, &vendor) < 0) - goto cleanup; + return NULL;
if (virFileReadAll(model_path, 1024, &model) < 0) - goto cleanup; + return NULL;
virTrimSpaces(vendor, NULL); virTrimSpaces(model, NULL);
if (virAsprintf(&dev->id, "%s:%s", vendor, model) < 0) - goto cleanup; + return NULL;
ret = dev; - cleanup: - if (!ret) - virSCSIDeviceFree(dev); + dev = NULL;
I'd suggest using VIR_STEAL_PTR instead.
return ret; }
@@ -281,21 +280,17 @@ virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *drvname, const char *domname) { - virUsedByInfoPtr copy; + VIR_AUTOPTR(virUsedByInfo) copy = NULL; if (VIR_ALLOC(copy) < 0) return -1; if (VIR_STRDUP(copy->drvname, drvname) < 0 || VIR_STRDUP(copy->domname, domname) < 0) - goto cleanup; + return -1;
if (VIR_APPEND_ELEMENT(dev->used_by, dev->n_used_by, copy) < 0) - goto cleanup; + return -1;
return 0; - - cleanup: - virSCSIDeviceUsedByInfoFree(copy); - return -1; }
bool @@ -442,7 +437,6 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list, const char *drvname, const char *domname) { - virSCSIDevicePtr tmp = NULL; size_t i;
for (i = 0; i < dev->n_used_by; i++) { @@ -452,8 +446,8 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list, virSCSIDeviceUsedByInfoFree(dev->used_by[i]); VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); } else { - tmp = virSCSIDeviceListSteal(list, dev); - virSCSIDeviceFree(tmp); + VIR_AUTOPTR(virSCSIDevice) tmp = + virSCSIDeviceListSteal(list, dev);
No need to save the lines that much, simply declare the variable on a separate line: VIR_AUTOPTR(virSCSIDevice) tmp = NULL; tmp = virSCSIDeviceListSteal(list, dev); With that: Reviewed-by: Erik Skultety <eskultet@redhat.com>
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header. When a variable of type virSCSIVHostDevicePtr is declared using VIR_AUTOPTR, the function virSCSIVHostDeviceFree will be run automatically on it when it goes out of scope. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virscsivhost.c | 1 - src/util/virscsivhost.h | 3 +++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c index 84c09e8..ef216b3 100644 --- a/src/util/virscsivhost.c +++ b/src/util/virscsivhost.c @@ -26,7 +26,6 @@ #include "virscsivhost.h" #include "virlog.h" -#include "viralloc.h" #include "virerror.h" #include "virfile.h" #include "virstring.h" diff --git a/src/util/virscsivhost.h b/src/util/virscsivhost.h index 21887dd..31ad429 100644 --- a/src/util/virscsivhost.h +++ b/src/util/virscsivhost.h @@ -27,6 +27,7 @@ # include "internal.h" # include "virobject.h" # include "virutil.h" +# include "viralloc.h" typedef struct _virSCSIVHostDevice virSCSIVHostDevice; typedef virSCSIVHostDevice *virSCSIVHostDevicePtr; @@ -63,4 +64,6 @@ void virSCSIVHostDeviceGetUsedBy(virSCSIVHostDevicePtr dev, void virSCSIVHostDeviceFree(virSCSIVHostDevicePtr dev); int virSCSIVHostOpenVhostSCSI(int *vhostfd) ATTRIBUTE_NOINLINE; +VIR_DEFINE_AUTOPTR_FUNC(virSCSIVHostDevice, virSCSIVHostDeviceFree) + #endif /* __VIR_SCSIHOST_H__ */ -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:01PM +0530, Sukrit Bhatnagar wrote:
Using the new VIR_DEFINE_AUTOPTR_FUNC macro defined in src/util/viralloc.h, define a new wrapper around an existing cleanup function which will be called when a variable declared with VIR_AUTOPTR macro goes out of scope. Also, drop the redundant viralloc.h include, since that has moved from the source module into the header.
When a variable of type virSCSIVHostDevicePtr is declared using VIR_AUTOPTR, the function virSCSIVHostDeviceFree will be run automatically on it when it goes out of scope.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virscsivhost.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c index ef216b3..3cd421e 100644 --- a/src/util/virscsivhost.c +++ b/src/util/virscsivhost.c @@ -109,8 +109,7 @@ void virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list, virSCSIVHostDevicePtr dev) { - virSCSIVHostDevicePtr tmp = virSCSIVHostDeviceListSteal(list, dev); - virSCSIVHostDeviceFree(tmp); + VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list, dev); } @@ -271,13 +270,11 @@ virSCSIVHostDeviceNew(const char *name) VIR_DEBUG("%s: initialized", dev->name); - cleanup: return dev; error: virSCSIVHostDeviceFree(dev); - dev = NULL; - goto cleanup; + return NULL; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:02PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virscsivhost.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c index ef216b3..3cd421e 100644 --- a/src/util/virscsivhost.c +++ b/src/util/virscsivhost.c @@ -109,8 +109,7 @@ void virSCSIVHostDeviceListDel(virSCSIVHostDeviceListPtr list, virSCSIVHostDevicePtr dev) { - virSCSIVHostDevicePtr tmp = virSCSIVHostDeviceListSteal(list, dev); - virSCSIVHostDeviceFree(tmp); + VIR_AUTOPTR(virSCSIVHostDevice) tmp = virSCSIVHostDeviceListSteal(list, dev); }
@@ -271,13 +270,11 @@ virSCSIVHostDeviceNew(const char *name)
VIR_DEBUG("%s: initialized", dev->name);
- cleanup: return dev;
error: virSCSIVHostDeviceFree(dev); - dev = NULL; - goto cleanup; + return NULL;
If you declared @dev as VIR_AUTOPTR along with a non-VIR_AUTOPTR @ret and then used VIR_STEAL_PTR and returned @ret instead, then you could get rid of all the goto labels in ^this function. With that tiny tweak: Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhostdev.c | 91 +++++++++++++++++---------------------------------- 1 file changed, 30 insertions(+), 61 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f4bd19d..c4335a9 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -186,17 +186,14 @@ virHostdevManagerNew(void) goto error; } } else { - char *rundir = NULL; + VIR_AUTOFREE(char *) rundir = NULL; mode_t old_umask; if (!(rundir = virGetUserRuntimeDirectory())) goto error; - if (virAsprintf(&hostdevMgr->stateDir, "%s/hostdevmgr", rundir) < 0) { - VIR_FREE(rundir); + if (virAsprintf(&hostdevMgr->stateDir, "%s/hostdevmgr", rundir) < 0) goto error; - } - VIR_FREE(rundir); old_umask = umask(077); @@ -289,17 +286,12 @@ virHostdevPCISysfsPath(virDomainHostdevDefPtr hostdev, static int virHostdevIsVirtualFunction(virDomainHostdevDefPtr hostdev) { - char *sysfs_path = NULL; - int ret = -1; + VIR_AUTOFREE(char *) sysfs_path = NULL; if (virHostdevPCISysfsPath(hostdev, &sysfs_path) < 0) - return ret; + return -1; - ret = virPCIIsVirtualFunction(sysfs_path); - - VIR_FREE(sysfs_path); - - return ret; + return virPCIIsVirtualFunction(sysfs_path); } @@ -309,17 +301,15 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev, char **linkdev, int *vf) { - int ret = -1; - char *sysfs_path = NULL; + VIR_AUTOFREE(char *) sysfs_path = NULL; if (virHostdevPCISysfsPath(hostdev, &sysfs_path) < 0) - return ret; + return -1; if (virPCIIsVirtualFunction(sysfs_path) == 1) { if (virPCIGetVirtualFunctionInfo(sysfs_path, pfNetDevIdx, - linkdev, vf) < 0) { - goto cleanup; - } + linkdev, vf) < 0) + return -1; } else { /* In practice this should never happen, since we currently * only support assigning SRIOV VFs via <interface @@ -327,24 +317,19 @@ virHostdevNetDevice(virDomainHostdevDefPtr hostdev, * end up calling this function. */ if (virPCIGetNetName(sysfs_path, 0, NULL, linkdev) < 0) - goto cleanup; + return -1; if (!linkdev) { virReportError(VIR_ERR_INTERNAL_ERROR, _("The device at %s has no network device name"), sysfs_path); - goto cleanup; + return -1; } *vf = -1; } - ret = 0; - - cleanup: - VIR_FREE(sysfs_path); - - return ret; + return 0; } @@ -443,8 +428,7 @@ static int virHostdevSaveNetConfig(virDomainHostdevDefPtr hostdev, const char *stateDir) { - int ret = -1; - char *linkdev = NULL; + VIR_AUTOFREE(char *) linkdev = NULL; int vf = -1; if (!virHostdevIsPCINetDevice(hostdev) || @@ -455,19 +439,16 @@ virHostdevSaveNetConfig(virDomainHostdevDefPtr hostdev, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Interface type hostdev is currently supported on" " SR-IOV Virtual Functions only")); - goto cleanup; + return -1; } if (virHostdevNetDevice(hostdev, -1, &linkdev, &vf) < 0) - goto cleanup; + return -1; if (virNetDevSaveNetConfig(linkdev, vf, stateDir, true) < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - VIR_FREE(linkdev); - return ret; + return 0; } @@ -486,10 +467,9 @@ static int virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev, const unsigned char *uuid) { - char *linkdev = NULL; + VIR_AUTOFREE(char *) linkdev = NULL; virNetDevVlanPtr vlan; virNetDevVPortProfilePtr virtPort; - int ret = -1; int vf = -1; bool port_profile_associate = true; @@ -497,7 +477,7 @@ virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev, return 0; if (virHostdevNetDevice(hostdev, -1, &linkdev, &vf) < 0) - goto cleanup; + return -1; vlan = virDomainNetGetActualVlan(hostdev->parent.data.net); virtPort = virDomainNetGetActualVirtPortProfile(hostdev->parent.data.net); @@ -507,24 +487,19 @@ virHostdevSetNetConfig(virDomainHostdevDefPtr hostdev, _("direct setting of the vlan tag is not allowed " "for hostdev devices using %s mode"), virNetDevVPortTypeToString(virtPort->virtPortType)); - goto cleanup; + return -1; } if (virHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort, &hostdev->parent.data.net->mac, - uuid, port_profile_associate) < 0) { - goto cleanup; - } + uuid, port_profile_associate) < 0) + return -1; } else { if (virNetDevSetNetConfig(linkdev, vf, &hostdev->parent.data.net->mac, - vlan, NULL, true) < 0) { - goto cleanup; - } + vlan, NULL, true) < 0) + return -1; } - ret = 0; - cleanup: - VIR_FREE(linkdev); - return ret; + return 0; } @@ -540,13 +515,13 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, const char *stateDir, const char *oldStateDir) { - char *linkdev = NULL; + VIR_AUTOFREE(char *) linkdev = NULL; + VIR_AUTOFREE(virMacAddrPtr) MAC = NULL; + VIR_AUTOFREE(virMacAddrPtr) adminMAC = NULL; virNetDevVPortProfilePtr virtPort; int ret = -1; int vf = -1; bool port_profile_associate = false; - virMacAddrPtr MAC = NULL; - virMacAddrPtr adminMAC = NULL; virNetDevVlanPtr vlan = NULL; @@ -656,9 +631,6 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, } cleanup: - VIR_FREE(linkdev); - VIR_FREE(MAC); - VIR_FREE(adminMAC); virNetDevVlanFree(vlan); return ret; @@ -763,8 +735,8 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, mgr->inactivePCIHostdevs) < 0) goto reattachdevs; } else { - char *driverPath; - char *driverName; + VIR_AUTOFREE(char *) driverPath = NULL; + VIR_AUTOFREE(char *) driverName = NULL; int stub; /* Unmanaged devices should already have been marked as @@ -790,9 +762,6 @@ virHostdevPreparePCIDevices(virHostdevManagerPtr mgr, stub = virPCIStubDriverTypeFromString(driverName); - VIR_FREE(driverPath); - VIR_FREE(driverName); - if (stub > VIR_PCI_STUB_DRIVER_NONE && stub < VIR_PCI_STUB_DRIVER_LAST) { -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:03PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhostdev.c | 71 ++++++++++++++++++--------------------------------- 1 file changed, 25 insertions(+), 46 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index c4335a9..b5315b1 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -518,11 +518,10 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, VIR_AUTOFREE(char *) linkdev = NULL; VIR_AUTOFREE(virMacAddrPtr) MAC = NULL; VIR_AUTOFREE(virMacAddrPtr) adminMAC = NULL; + VIR_AUTOPTR(virNetDevVlan) vlan = NULL; virNetDevVPortProfilePtr virtPort; - int ret = -1; int vf = -1; bool port_profile_associate = false; - virNetDevVlanPtr vlan = NULL; /* This is only needed for PCI devices that have been defined @@ -535,16 +534,16 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Interface type hostdev is currently supported on" " SR-IOV Virtual Functions only")); - return ret; + return -1; } if (virHostdevNetDevice(hostdev, 0, &linkdev, &vf) < 0) - return ret; + return -1; virtPort = virDomainNetGetActualVirtPortProfile( hostdev->parent.data.net); if (virtPort) { - ret = virHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort, + return virHostdevNetConfigVirtPortProfile(linkdev, vf, virtPort, &hostdev->parent.data.net->mac, NULL, port_profile_associate); @@ -574,7 +573,7 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, /* 1) standard location */ if (virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &vlan, &MAC) < 0) { - goto cleanup; + return -1; } /* 2) "old" (pre-1.2.3 circa 2014) location - whenever we get @@ -585,7 +584,7 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, if (!(adminMAC || vlan || MAC) && oldStateDir && virNetDevReadNetConfig(linkdev, vf, oldStateDir, &adminMAC, &vlan, &MAC) < 0) { - goto cleanup; + return -1; } /* 3) try using the PF's "port 2" netdev as the name of the @@ -597,7 +596,7 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, if (virHostdevNetDevice(hostdev, 1, &linkdev, &vf) < 0 || virNetDevReadNetConfig(linkdev, vf, stateDir, &adminMAC, &vlan, &MAC) < 0) { - goto cleanup; + return -1; } } @@ -627,13 +626,8 @@ virHostdevRestoreNetConfig(virDomainHostdevDefPtr hostdev, ignore_value(virNetDevSetNetConfig(linkdev, vf, adminMAC, vlan, MAC, true)); - ret = 0; + return 0; } - - cleanup: - virNetDevVlanFree(vlan); - - return ret; } int @@ -1117,7 +1111,6 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, const char *dom_name) { virDomainHostdevDefPtr hostdev = NULL; - virPCIDevicePtr actual = NULL; size_t i; int ret = -1; @@ -1128,6 +1121,7 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, virObjectLock(mgr->inactivePCIHostdevs); for (i = 0; i < nhostdevs; i++) { + VIR_AUTOPTR(virPCIDevice) actual = NULL; virDomainHostdevSubsysPCIPtr pcisrc; hostdev = hostdevs[i]; pcisrc = &hostdev->source.subsys.u.pci; @@ -1165,7 +1159,6 @@ virHostdevUpdateActivePCIDevices(virHostdevManagerPtr mgr, ret = 0; cleanup: - virPCIDeviceFree(actual); virObjectUnlock(mgr->activePCIHostdevs); virObjectUnlock(mgr->inactivePCIHostdevs); return ret; @@ -1226,31 +1219,27 @@ virHostdevUpdateActiveSCSIHostDevices(virHostdevManagerPtr mgr, virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virSCSIDevicePtr scsi = NULL; virSCSIDevicePtr tmp = NULL; - int ret = -1; if (!(scsi = virSCSIDeviceNew(NULL, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, hostdev->readonly, hostdev->shareable))) - goto cleanup; + return -1; if ((tmp = virSCSIDeviceListFind(mgr->activeSCSIHostdevs, scsi))) { if (virSCSIDeviceSetUsedBy(tmp, drv_name, dom_name) < 0) { virSCSIDeviceFree(scsi); - goto cleanup; + return -1; } virSCSIDeviceFree(scsi); } else { if (virSCSIDeviceSetUsedBy(scsi, drv_name, dom_name) < 0 || virSCSIDeviceListAdd(mgr->activeSCSIHostdevs, scsi) < 0) { virSCSIDeviceFree(scsi); - goto cleanup; + return -1; } } - ret = 0; - - cleanup: - return ret; + return 0; } int @@ -1301,7 +1290,7 @@ virHostdevUpdateActiveMediatedDevices(virHostdevManagerPtr mgr, { int ret = -1; size_t i; - virMediatedDevicePtr mdev = NULL; + VIR_AUTOPTR(virMediatedDevice) mdev = NULL; if (nhostdevs == 0) return 0; @@ -1327,7 +1316,6 @@ virHostdevUpdateActiveMediatedDevices(virHostdevManagerPtr mgr, ret = 0; cleanup: - virMediatedDeviceFree(mdev); virObjectUnlock(mgr->activeMediatedHostdevs); return ret; } @@ -1560,29 +1548,25 @@ virHostdevPrepareSCSIHostDevices(virDomainHostdevDefPtr hostdev, { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; virSCSIDevicePtr scsi; - int ret = -1; if (hostdev->managed) { virReportError(VIR_ERR_XML_ERROR, "%s", _("SCSI host device doesn't support managed mode")); - goto cleanup; + return -1; } if (!(scsi = virSCSIDeviceNew(NULL, scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit, hostdev->readonly, hostdev->shareable))) - goto cleanup; + return -1; if (virSCSIDeviceListAdd(list, scsi) < 0) { virSCSIDeviceFree(scsi); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - return ret; + return 0; } int @@ -1859,7 +1843,8 @@ virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr, for (i = 0; i < nhostdevs; i++) { virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysUSBPtr usbsrc = &hostdev->source.subsys.u.usb; - virUSBDevicePtr usb, tmp; + VIR_AUTOPTR(virUSBDevice) usb = NULL; + virUSBDevicePtr tmp; const char *usedby_drvname; const char *usedby_domname; @@ -1883,7 +1868,6 @@ virHostdevReAttachUSBDevices(virHostdevManagerPtr mgr, * the list which were taken by @name */ tmp = virUSBDeviceListFind(mgr->activeUSBHostdevs, usb); - virUSBDeviceFree(usb); if (!tmp) { VIR_WARN("Unable to find device %03d.%03d " @@ -1911,7 +1895,7 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr mgr, const char *dom_name) { virDomainHostdevSubsysSCSIHostPtr scsihostsrc = &scsisrc->u.host; - virSCSIDevicePtr scsi; + VIR_AUTOPTR(virSCSIDevice) scsi = NULL; virSCSIDevicePtr tmp; if (!(scsi = virSCSIDeviceNew(NULL, @@ -1932,7 +1916,6 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr mgr, "in list of active SCSI devices", scsihostsrc->adapter, scsihostsrc->bus, scsihostsrc->target, scsihostsrc->unit); - virSCSIDeviceFree(scsi); return; } @@ -1942,7 +1925,6 @@ virHostdevReAttachSCSIHostDevices(virHostdevManagerPtr mgr, virSCSIDeviceListDel(mgr->activeSCSIHostdevs, tmp, drv_name, dom_name); - virSCSIDeviceFree(scsi); } void @@ -1982,14 +1964,14 @@ virHostdevReAttachSCSIVHostDevices(virHostdevManagerPtr mgr, int nhostdevs) { size_t i; - virSCSIVHostDevicePtr host, tmp; - if (!nhostdevs) return; virObjectLock(mgr->activeSCSIVHostHostdevs); for (i = 0; i < nhostdevs; i++) { + VIR_AUTOPTR(virSCSIVHostDevice) host = NULL; + virSCSIVHostDevicePtr tmp; virDomainHostdevDefPtr hostdev = hostdevs[i]; virDomainHostdevSubsysSCSIVHostPtr hostsrc = &hostdev->source.subsys.u.scsi_host; const char *usedby_drvname; @@ -2017,7 +1999,6 @@ virHostdevReAttachSCSIVHostDevices(virHostdevManagerPtr mgr, VIR_WARN("Unable to find device %s " "in list of active SCSI_host devices", hostsrc->wwpn); - virSCSIVHostDeviceFree(host); virObjectUnlock(mgr->activeSCSIVHostHostdevs); return; } @@ -2031,8 +2012,6 @@ virHostdevReAttachSCSIVHostDevices(virHostdevManagerPtr mgr, virSCSIVHostDeviceListDel(mgr->activeSCSIVHostHostdevs, tmp); } - - virSCSIVHostDeviceFree(host); } virObjectUnlock(mgr->activeSCSIVHostHostdevs); } @@ -2060,7 +2039,8 @@ virHostdevReAttachMediatedDevices(virHostdevManagerPtr mgr, virObjectLock(mgr->activeMediatedHostdevs); for (i = 0; i < nhostdevs; i++) { - virMediatedDevicePtr mdev, tmp; + VIR_AUTOPTR(virMediatedDevice) mdev = NULL; + virMediatedDevicePtr tmp; virDomainHostdevSubsysMediatedDevPtr mdevsrc; virDomainHostdevDefPtr hostdev = hostdevs[i]; @@ -2076,7 +2056,6 @@ virHostdevReAttachMediatedDevices(virHostdevManagerPtr mgr, /* Remove from the list only mdevs assigned to @drv_name/@dom_name */ tmp = virMediatedDeviceListFind(mgr->activeMediatedHostdevs, mdev); - virMediatedDeviceFree(mdev); /* skip inactive devices */ if (!tmp) -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:04PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhostmem.c | 57 ++++++++++++++++----------------------------------- 1 file changed, 18 insertions(+), 39 deletions(-) diff --git a/src/util/virhostmem.c b/src/util/virhostmem.c index c923a1e..08ed7a5 100644 --- a/src/util/virhostmem.c +++ b/src/util/virhostmem.c @@ -263,7 +263,7 @@ virHostMemGetStats(int cellNum ATTRIBUTE_UNUSED, #ifdef __linux__ { int ret; - char *meminfo_path = NULL; + VIR_AUTOFREE(char *) meminfo_path = NULL; FILE *meminfo; int max_node; @@ -299,12 +299,10 @@ virHostMemGetStats(int cellNum ATTRIBUTE_UNUSED, if (!meminfo) { virReportSystemError(errno, _("cannot open %s"), meminfo_path); - VIR_FREE(meminfo_path); return -1; } ret = virHostMemGetStatsLinux(meminfo, cellNum, params, nparams); VIR_FORCE_FCLOSE(meminfo); - VIR_FREE(meminfo_path); return ret; } @@ -322,45 +320,36 @@ virHostMemGetStats(int cellNum ATTRIBUTE_UNUSED, static int virHostMemSetParameterValue(virTypedParameterPtr param) { - char *path = NULL; - char *strval = NULL; - int ret = -1; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) strval = NULL; int rc = -1; char *field = strchr(param->field, '_'); sa_assert(field); field++; if (virAsprintf(&path, "%s/%s", - SYSFS_MEMORY_SHARED_PATH, field) < 0) { - ret = -2; - goto cleanup; - } + SYSFS_MEMORY_SHARED_PATH, field) < 0) + return -2; - if (virAsprintf(&strval, "%u", param->value.ui) == -1) { - ret = -2; - goto cleanup; - } + if (virAsprintf(&strval, "%u", param->value.ui) == -1) + return -2; if ((rc = virFileWriteStr(path, strval, 0)) < 0) { virReportSystemError(-rc, _("failed to set %s"), param->field); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(path); - VIR_FREE(strval); - return ret; + return 0; } static bool virHostMemParametersAreAllSupported(virTypedParameterPtr params, int nparams) { - char *path = NULL; size_t i; for (i = 0; i < nparams; i++) { + VIR_AUTOFREE(char *) path = NULL; virTypedParameterPtr param = ¶ms[i]; char *field = strchr(param->field, '_'); @@ -374,11 +363,8 @@ virHostMemParametersAreAllSupported(virTypedParameterPtr params, virReportError(VIR_ERR_OPERATION_INVALID, _("Parameter '%s' is not supported by " "this kernel"), param->field); - VIR_FREE(path); return false; } - - VIR_FREE(path); } return true; @@ -430,23 +416,20 @@ static int virHostMemGetParameterValue(const char *field, void *value) { - char *path = NULL; - char *buf = NULL; + VIR_AUTOFREE(char *) path = NULL; + VIR_AUTOFREE(char *) buf = NULL; char *tmp = NULL; - int ret = -1; int rc = -1; if (virAsprintf(&path, "%s/%s", SYSFS_MEMORY_SHARED_PATH, field) < 0) - goto cleanup; + return -1; - if (!virFileExists(path)) { - ret = -2; - goto cleanup; - } + if (!virFileExists(path)) + return -2; if (virFileReadAll(path, 1024, &buf) < 0) - goto cleanup; + return -1; if ((tmp = strchr(buf, '\n'))) *tmp = '\0'; @@ -465,14 +448,10 @@ virHostMemGetParameterValue(const char *field, if (rc < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to parse %s"), field); - goto cleanup; + return -1; } - ret = 0; - cleanup: - VIR_FREE(path); - VIR_FREE(buf); - return ret; + return 0; } #endif -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:05PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/viriptables.c | 52 +++++++++++++++++--------------------------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/src/util/viriptables.c b/src/util/viriptables.c index e921954..e65e8dc 100644 --- a/src/util/viriptables.c +++ b/src/util/viriptables.c @@ -215,7 +215,7 @@ static char *iptablesFormatNetwork(virSocketAddr *netaddr, unsigned int prefix) { virSocketAddr network; - char *netstr; + VIR_AUTOFREE(char *) netstr = NULL; char *ret; if (!(VIR_SOCKET_ADDR_IS_FAMILY(netaddr, AF_INET) || @@ -238,7 +238,6 @@ static char *iptablesFormatNetwork(virSocketAddr *netaddr, ignore_value(virAsprintf(&ret, "%s/%d", netstr, prefix)); - VIR_FREE(netstr); return ret; } @@ -254,7 +253,7 @@ iptablesForwardAllowOut(virFirewallPtr fw, const char *physdev, int action) { - char *networkstr; + VIR_AUTOFREE(char *) networkstr = NULL; virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; @@ -279,7 +278,6 @@ iptablesForwardAllowOut(virFirewallPtr fw, "--jump", "ACCEPT", NULL); - VIR_FREE(networkstr); return 0; } @@ -343,7 +341,7 @@ iptablesForwardAllowRelatedIn(virFirewallPtr fw, { virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - char *networkstr; + VIR_AUTOFREE(char *) networkstr = NULL; if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) return -1; @@ -370,7 +368,6 @@ iptablesForwardAllowRelatedIn(virFirewallPtr fw, "--jump", "ACCEPT", NULL); - VIR_FREE(networkstr); return 0; } @@ -432,7 +429,7 @@ iptablesForwardAllowIn(virFirewallPtr fw, { virFirewallLayer layer = VIR_SOCKET_ADDR_FAMILY(netaddr) == AF_INET ? VIR_FIREWALL_LAYER_IPV4 : VIR_FIREWALL_LAYER_IPV6; - char *networkstr; + VIR_AUTOFREE(char *) networkstr = NULL; if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) return -1; @@ -454,7 +451,6 @@ iptablesForwardAllowIn(virFirewallPtr fw, "--out-interface", iface, "--jump", "ACCEPT", NULL); - VIR_FREE(networkstr); return 0; } @@ -661,12 +657,11 @@ iptablesForwardMasquerade(virFirewallPtr fw, const char *protocol, int action) { - int ret = -1; - char *networkstr = NULL; - char *addrStartStr = NULL; - char *addrEndStr = NULL; - char *portRangeStr = NULL; - char *natRangeStr = NULL; + VIR_AUTOFREE(char *) networkstr = NULL; + VIR_AUTOFREE(char *) addrStartStr = NULL; + VIR_AUTOFREE(char *) addrEndStr = NULL; + VIR_AUTOFREE(char *) portRangeStr = NULL; + VIR_AUTOFREE(char *) natRangeStr = NULL; virFirewallRulePtr rule; if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) @@ -677,15 +672,15 @@ iptablesForwardMasquerade(virFirewallPtr fw, virReportError(VIR_ERR_INTERNAL_ERROR, _("Attempted to NAT '%s'. NAT is only supported for IPv4."), networkstr); - goto cleanup; + return -1; } if (VIR_SOCKET_ADDR_IS_FAMILY(&addr->start, AF_INET)) { if (!(addrStartStr = virSocketAddrFormat(&addr->start))) - goto cleanup; + return -1; if (VIR_SOCKET_ADDR_IS_FAMILY(&addr->end, AF_INET)) { if (!(addrEndStr = virSocketAddrFormat(&addr->end))) - goto cleanup; + return -1; } } @@ -718,7 +713,7 @@ iptablesForwardMasquerade(virFirewallPtr fw, if (port->start < port->end && port->end < 65536) { if (virAsprintf(&portRangeStr, ":%u-%u", port->start, port->end) < 0) - goto cleanup; + return -1; } else { virReportError(VIR_ERR_INTERNAL_ERROR, _("Invalid port range '%u-%u'."), @@ -739,7 +734,7 @@ iptablesForwardMasquerade(virFirewallPtr fw, } if (r < 0) - goto cleanup; + return -1; virFirewallRuleAddArgList(fw, rule, "--jump", "SNAT", @@ -753,14 +748,7 @@ iptablesForwardMasquerade(virFirewallPtr fw, "--to-ports", &portRangeStr[1], NULL); } - ret = 0; - cleanup: - VIR_FREE(networkstr); - VIR_FREE(addrStartStr); - VIR_FREE(addrEndStr); - VIR_FREE(portRangeStr); - VIR_FREE(natRangeStr); - return ret; + return 0; } /** @@ -827,8 +815,7 @@ iptablesForwardDontMasquerade(virFirewallPtr fw, const char *destaddr, int action) { - int ret = -1; - char *networkstr = NULL; + VIR_AUTOFREE(char *) networkstr = NULL; if (!(networkstr = iptablesFormatNetwork(netaddr, prefix))) return -1; @@ -838,7 +825,7 @@ iptablesForwardDontMasquerade(virFirewallPtr fw, virReportError(VIR_ERR_INTERNAL_ERROR, _("Attempted to NAT '%s'. NAT is only supported for IPv4."), networkstr); - goto cleanup; + return -1; } if (physdev && physdev[0]) @@ -859,10 +846,7 @@ iptablesForwardDontMasquerade(virFirewallPtr fw, "--jump", "RETURN", NULL); - ret = 0; - cleanup: - VIR_FREE(networkstr); - return ret; + return 0; } /** -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:06PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/viriscsi.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index d4c745a..13fd02c 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -80,7 +80,7 @@ virISCSIGetSession(const char *devpath, .session = NULL, .devpath = devpath, }; - char *error = NULL; + VIR_AUTOFREE(char *) error = NULL; int exitstatus = 0; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", @@ -101,7 +101,6 @@ virISCSIGetSession(const char *devpath, NULLSTR(error)); cleanup: - VIR_FREE(error); virCommandFree(cmd); return cbdata.session; } @@ -120,7 +119,10 @@ virStorageBackendIQNFound(const char *initiatoriqn, int ret = IQN_MISSING, fd = -1; char ebuf[64]; FILE *fp = NULL; - char *line = NULL, *newline = NULL, *iqn = NULL, *token = NULL; + VIR_AUTOFREE(char *) line = NULL; + char *newline = NULL; + char *iqn = NULL; + char *token = NULL; virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", "iface", NULL); @@ -192,7 +194,6 @@ virStorageBackendIQNFound(const char *initiatoriqn, if (ret == IQN_MISSING) VIR_DEBUG("Could not find interface with IQN '%s'", iqn); - VIR_FREE(line); VIR_FORCE_FCLOSE(fp); VIR_FORCE_CLOSE(fd); virCommandFree(cmd); @@ -206,7 +207,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, char **ifacename) { int ret = -1, exitstatus = -1; - char *temp_ifacename; + VIR_AUTOFREE(char *) temp_ifacename = NULL; virCommandPtr cmd = NULL; if (virAsprintf(&temp_ifacename, @@ -267,7 +268,6 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, cleanup: virCommandFree(cmd); - VIR_FREE(temp_ifacename); if (ret != 0) VIR_FREE(*ifacename); return ret; @@ -289,7 +289,7 @@ virISCSIConnection(const char *portal, NULL }; virCommandPtr cmd; - char *ifacename = NULL; + VIR_AUTOFREE(char *) ifacename = NULL; cmd = virCommandNewArgs(baseargv); virCommandAddArgSet(cmd, extraargv); @@ -326,7 +326,6 @@ virISCSIConnection(const char *portal, cleanup: virCommandFree(cmd); - VIR_FREE(ifacename); return ret; } @@ -377,15 +376,13 @@ virISCSIGetTargets(char **const groups, void *data) { struct virISCSITargetList *list = data; - char *target; + VIR_AUTOFREE(char *) target = NULL; if (VIR_STRDUP(target, groups[1]) < 0) return -1; - if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0) { - VIR_FREE(target); + if (VIR_APPEND_ELEMENT(list->targets, list->ntargets, target) < 0) return -1; - } return 0; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:07PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> ---
Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/viriscsi.c | 68 +++++++++++++++++------------------------------------ 1 file changed, 22 insertions(+), 46 deletions(-) diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 13fd02c..a3367bc 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -83,7 +83,7 @@ virISCSIGetSession(const char *devpath, VIR_AUTOFREE(char *) error = NULL; int exitstatus = 0; - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", + VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); virCommandSetErrorBuffer(cmd, &error); @@ -93,15 +93,13 @@ virISCSIGetSession(const char *devpath, vars, virISCSIExtractSession, &cbdata, NULL, &exitstatus) < 0) - goto cleanup; + return cbdata.session; if (cbdata.session == NULL && !probe) virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot find iscsiadm session: %s"), NULLSTR(error)); - cleanup: - virCommandFree(cmd); return cbdata.session; } @@ -123,7 +121,7 @@ virStorageBackendIQNFound(const char *initiatoriqn, char *newline = NULL; char *iqn = NULL; char *token = NULL; - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, + VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode", "iface", NULL); if (VIR_ALLOC_N(line, LINE_SIZE) != 0) { @@ -196,7 +194,6 @@ virStorageBackendIQNFound(const char *initiatoriqn, VIR_FORCE_FCLOSE(fp); VIR_FORCE_CLOSE(fd); - virCommandFree(cmd); return ret; } @@ -208,7 +205,7 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, { int ret = -1, exitstatus = -1; VIR_AUTOFREE(char *) temp_ifacename = NULL; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; if (virAsprintf(&temp_ifacename, "libvirt-iface-%08llx", @@ -267,7 +264,6 @@ virStorageBackendCreateIfaceIQN(const char *initiatoriqn, ret = 0; cleanup: - virCommandFree(cmd); if (ret != 0) VIR_FREE(*ifacename); return ret; @@ -280,7 +276,6 @@ virISCSIConnection(const char *portal, const char *target, const char **extraargv) { - int ret = -1; const char *const baseargv[] = { ISCSIADM, "--mode", "node", @@ -288,7 +283,7 @@ virISCSIConnection(const char *portal, "--targetname", target, NULL }; - virCommandPtr cmd; + VIR_AUTOPTR(virCommand) cmd = NULL; VIR_AUTOFREE(char *) ifacename = NULL; cmd = virCommandNewArgs(baseargv); @@ -301,7 +296,7 @@ virISCSIConnection(const char *portal, break; case IQN_MISSING: if (virStorageBackendCreateIfaceIQN(initiatoriqn, &ifacename) != 0) - goto cleanup; + return -1; /* * iscsiadm doesn't let you send commands to the Interface IQN, * unless you've first issued a 'sendtargets' command to the @@ -309,25 +304,20 @@ virISCSIConnection(const char *portal, * "iscsiadm: No records found" */ if (virISCSIScanTargets(portal, NULL, NULL) < 0) - goto cleanup; + return -1; break; case IQN_ERROR: default: - goto cleanup; + return -1; } virCommandAddArgList(cmd, "--interface", ifacename, NULL); } if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virCommandFree(cmd); - - return ret; + return 0; } @@ -354,14 +344,12 @@ virISCSIConnectionLogout(const char *portal, int virISCSIRescanLUNs(const char *session) { - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, + VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", "-r", session, "-R", NULL); - int ret = virCommandRun(cmd, NULL); - virCommandFree(cmd); - return ret; + return virCommandRun(cmd, NULL); } @@ -409,8 +397,7 @@ virISCSIScanTargets(const char *portal, int vars[] = { 2 }; struct virISCSITargetList list; size_t i; - int ret = -1; - virCommandPtr cmd = virCommandNewArgList(ISCSIADM, + VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode", "discovery", "--type", "sendtargets", "--portal", portal, @@ -425,7 +412,7 @@ virISCSIScanTargets(const char *portal, vars, virISCSIGetTargets, &list, NULL, NULL) < 0) - goto cleanup; + return -1; if (ntargetsret && targetsret) { *ntargetsret = list.ntargets; @@ -436,10 +423,7 @@ virISCSIScanTargets(const char *portal, VIR_FREE(list.targets); } - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return 0; } /* @@ -462,9 +446,8 @@ int virISCSINodeNew(const char *portal, const char *target) { - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int status; - int ret = -1; cmd = virCommandNewArgList(ISCSIADM, "--mode", "node", @@ -477,20 +460,17 @@ virISCSINodeNew(const char *portal, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed new node mode for target '%s'"), target); - goto cleanup; + return -1; } if (status != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("%s failed new mode for target '%s' with status '%d'"), ISCSIADM, target, status); - goto cleanup; + return -1; } - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return 0; } @@ -500,9 +480,8 @@ virISCSINodeUpdate(const char *portal, const char *name, const char *value) { - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; int status; - int ret = -1; cmd = virCommandNewArgList(ISCSIADM, "--mode", "node", @@ -518,11 +497,8 @@ virISCSINodeUpdate(const char *portal, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to update '%s' of node mode for target '%s'"), name, target); - goto cleanup; + return -1; } - ret = 0; - cleanup: - virCommandFree(cmd); - return ret; + return 0; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:08PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/viriscsi.c | 68 +++++++++++++++++------------------------------------ 1 file changed, 22 insertions(+), 46 deletions(-)
diff --git a/src/util/viriscsi.c b/src/util/viriscsi.c index 13fd02c..a3367bc 100644 --- a/src/util/viriscsi.c +++ b/src/util/viriscsi.c @@ -83,7 +83,7 @@ virISCSIGetSession(const char *devpath, VIR_AUTOFREE(char *) error = NULL; int exitstatus = 0;
- virCommandPtr cmd = virCommandNewArgList(ISCSIADM, "--mode", + VIR_AUTOPTR(virCommand) cmd = virCommandNewArgList(ISCSIADM, "--mode", "session", NULL); virCommandSetErrorBuffer(cmd, &error);
@@ -93,15 +93,13 @@ virISCSIGetSession(const char *devpath, vars, virISCSIExtractSession, &cbdata, NULL, &exitstatus) < 0) - goto cleanup; + return cbdata.session;
I'd prefer you returned NULL explicitly to avoid any confusion. With that: Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virkmod.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/src/util/virkmod.c b/src/util/virkmod.c index 219fad6..d981cd4 100644 --- a/src/util/virkmod.c +++ b/src/util/virkmod.c @@ -155,13 +155,12 @@ virKModUnload(const char *module) bool virKModIsBlacklisted(const char *module) { - bool retval = false; size_t i; - char *drvblklst = NULL; - char *outbuf = NULL; + VIR_AUTOFREE(char *) drvblklst = NULL; + VIR_AUTOFREE(char *) outbuf = NULL; if (virAsprintfQuiet(&drvblklst, "blacklist %s\n", module) < 0) - goto cleanup; + return false; /* modprobe will convert all '-' into '_', so we need to as well */ for (i = 0; i < drvblklst[i]; i++) @@ -169,13 +168,10 @@ virKModIsBlacklisted(const char *module) drvblklst[i] = '_'; if (doModprobe("-c", NULL, &outbuf, NULL) < 0) - goto cleanup; + return false; if (strstr(outbuf, drvblklst)) - retval = true; + return true; - cleanup: - VIR_FREE(drvblklst); - VIR_FREE(outbuf); - return retval; + return false; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:09PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virkmod.c | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/src/util/virkmod.c b/src/util/virkmod.c index d981cd4..9d0375b 100644 --- a/src/util/virkmod.c +++ b/src/util/virkmod.c @@ -28,8 +28,7 @@ static int doModprobe(const char *opts, const char *module, char **outbuf, char **errbuf) { - int ret = -1; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNew(MODPROBE); if (opts) @@ -42,32 +41,23 @@ doModprobe(const char *opts, const char *module, char **outbuf, char **errbuf) virCommandSetErrorBuffer(cmd, errbuf); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virCommandFree(cmd); - return ret; + return 0; } static int doRmmod(const char *module, char **errbuf) { - int ret = -1; - virCommandPtr cmd = NULL; + VIR_AUTOPTR(virCommand) cmd = NULL; cmd = virCommandNewArgList(RMMOD, module, NULL); virCommandSetErrorBuffer(cmd, errbuf); if (virCommandRun(cmd, NULL) < 0) - goto cleanup; + return -1; - ret = 0; - - cleanup: - virCommandFree(cmd); - return ret; + return 0; } /** -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:10PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virlease.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/util/virlease.c b/src/util/virlease.c index b49105d..baaceaf 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -55,7 +55,7 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, const char *ip_to_delete, char **server_duid) { - char *lease_entries = NULL; + VIR_AUTOFREE(char *) lease_entries = NULL; virJSONValuePtr leases_array = NULL; long long expirytime; int custom_lease_file_len = 0; @@ -146,7 +146,6 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, cleanup: virJSONValueFree(leases_array); - VIR_FREE(lease_entries); return ret; } @@ -235,7 +234,7 @@ virLeaseNew(virJSONValuePtr *lease_ret, virJSONValuePtr lease_new = NULL; const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES"); long long expirytime = 0; - char *exptime = NULL; + VIR_AUTOFREE(char *) exptime = NULL; int ret = -1; /* In case hostname is still unknown, use the last known one */ @@ -291,7 +290,6 @@ virLeaseNew(virJSONValuePtr *lease_ret, *lease_ret = lease_new; lease_new = NULL; cleanup: - VIR_FREE(exptime); virJSONValueFree(lease_new); return ret; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:11PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOFREE macro for declaring scalar variables, majority of the VIR_FREE calls can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virlease.c | 76 ++++++++++++++++++++--------------------------------- 1 file changed, 29 insertions(+), 47 deletions(-) diff --git a/src/util/virlease.c b/src/util/virlease.c index baaceaf..7c6c37e 100644 --- a/src/util/virlease.c +++ b/src/util/virlease.c @@ -56,40 +56,36 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, char **server_duid) { VIR_AUTOFREE(char *) lease_entries = NULL; - virJSONValuePtr leases_array = NULL; + VIR_AUTOPTR(virJSONValue) leases_array = NULL; long long expirytime; int custom_lease_file_len = 0; virJSONValuePtr lease_tmp = NULL; const char *ip_tmp = NULL; const char *server_duid_tmp = NULL; size_t i; - int ret = -1; /* Read entire contents */ if ((custom_lease_file_len = virFileReadAll(custom_lease_file, VIR_NETWORK_DHCP_LEASE_FILE_SIZE_MAX, &lease_entries)) < 0) { - goto cleanup; + return -1; } /* Check for previous leases */ - if (custom_lease_file_len == 0) { - ret = 0; - goto cleanup; - } + if (custom_lease_file_len == 0) + return 0; if (!(leases_array = virJSONValueFromString(lease_entries))) { virReportError(VIR_ERR_INTERNAL_ERROR, _("invalid json in file: %s, rewriting it"), custom_lease_file); - ret = 0; - goto cleanup; + return 0; } if (!virJSONValueIsArray(leases_array)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("couldn't fetch array of leases")); - goto cleanup; + return -1; } i = 0; @@ -97,14 +93,14 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, if (!(lease_tmp = virJSONValueArrayGet(leases_array, i))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); - goto cleanup; + return -1; } if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address")) || (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", &expirytime) < 0)) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); - goto cleanup; + return -1; } /* Check whether lease has to be included or not */ @@ -121,14 +117,14 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, /* Control reaches here when the 'action' is not for an * ipv6 lease or, for some weird reason the env var * DNSMASQ_SERVER_DUID wasn't set*/ - goto cleanup; + return -1; } } else { /* Inject server-duid into those ipv6 leases which * didn't have it previously, for example, those * created by leaseshelper from libvirt 1.2.6 */ if (virJSONValueObjectAppendString(lease_tmp, "server-duid", *server_duid) < 0) - goto cleanup; + return -1; } } @@ -136,17 +132,13 @@ virLeaseReadCustomLeaseFile(virJSONValuePtr leases_array_new, if (virJSONValueArrayAppend(leases_array_new, lease_tmp) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to create json")); - goto cleanup; + return -1; } ignore_value(virJSONValueArraySteal(leases_array, i)); } - ret = 0; - - cleanup: - virJSONValueFree(leases_array); - return ret; + return 0; } @@ -157,7 +149,6 @@ virLeasePrintLeases(virJSONValuePtr leases_array_new, virJSONValuePtr lease_tmp = NULL; const char *ip_tmp = NULL; long long expirytime = 0; - int ret = -1; size_t i; /* Man page of dnsmasq says: the script (helper program, in our case) @@ -174,7 +165,7 @@ virLeasePrintLeases(virJSONValuePtr leases_array_new, if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); - goto cleanup; + return -1; } if (!strchr(ip_tmp, ':')) { if (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", @@ -198,7 +189,7 @@ virLeasePrintLeases(virJSONValuePtr leases_array_new, if (!(ip_tmp = virJSONValueObjectGetString(lease_tmp, "ip-address"))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to parse json")); - goto cleanup; + return -1; } if (strchr(ip_tmp, ':')) { if (virJSONValueObjectGetNumberLong(lease_tmp, "expiry-time", @@ -215,10 +206,7 @@ virLeasePrintLeases(virJSONValuePtr leases_array_new, } } - ret = 0; - - cleanup: - return ret; + return 0; } @@ -231,24 +219,21 @@ virLeaseNew(virJSONValuePtr *lease_ret, const char *iaid, const char *server_duid) { - virJSONValuePtr lease_new = NULL; + VIR_AUTOPTR(virJSONValue) lease_new = NULL; const char *exptime_tmp = virGetEnvAllowSUID("DNSMASQ_LEASE_EXPIRES"); long long expirytime = 0; VIR_AUTOFREE(char *) exptime = NULL; - int ret = -1; /* In case hostname is still unknown, use the last known one */ if (!hostname) hostname = virGetEnvAllowSUID("DNSMASQ_OLD_HOSTNAME"); - if (!mac) { - ret = 0; - goto cleanup; - } + if (!mac) + return 0; if (exptime_tmp) { if (VIR_STRDUP(exptime, exptime_tmp) < 0) - goto cleanup; + return -1; /* Removed extraneous trailing space in DNSMASQ_LEASE_EXPIRES * (dnsmasq < 2.52) */ @@ -261,35 +246,32 @@ virLeaseNew(virJSONValuePtr *lease_ret, virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to convert lease expiry time to long long: %s"), NULLSTR(exptime)); - goto cleanup; + return -1; } /* Create new lease */ if (!(lease_new = virJSONValueNewObject())) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to create json")); - goto cleanup; + return -1; } if (iaid && virJSONValueObjectAppendString(lease_new, "iaid", iaid) < 0) - goto cleanup; + return -1; if (ip && virJSONValueObjectAppendString(lease_new, "ip-address", ip) < 0) - goto cleanup; + return -1; if (mac && virJSONValueObjectAppendString(lease_new, "mac-address", mac) < 0) - goto cleanup; + return -1; if (hostname && virJSONValueObjectAppendString(lease_new, "hostname", hostname) < 0) - goto cleanup; + return -1; if (clientid && virJSONValueObjectAppendString(lease_new, "client-id", clientid) < 0) - goto cleanup; + return -1; if (server_duid && virJSONValueObjectAppendString(lease_new, "server-duid", server_duid) < 0) - goto cleanup; + return -1; if (expirytime && virJSONValueObjectAppendNumberLong(lease_new, "expiry-time", expirytime) < 0) - goto cleanup; + return -1; - ret = 0; *lease_ret = lease_new; lease_new = NULL; - cleanup: - virJSONValueFree(lease_new); - return ret; + return 0; } -- 1.8.3.1
On Sat, Jul 21, 2018 at 05:37:12PM +0530, Sukrit Bhatnagar wrote:
By making use of GNU C's cleanup attribute handled by the VIR_AUTOPTR macro for declaring aggregate pointer variables, majority of the calls to *Free functions can be dropped, which in turn leads to getting rid of most of our cleanup sections.
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- Reviewed-by: Erik Skultety <eskultet@redhat.com>
participants (3)
-
Daniel P. Berrangé -
Erik Skultety -
Sukrit Bhatnagar