[libvirt] [PATCH v2 00/41] 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 and that of virUSBDeviceListAdd is changed to take a double pointer to virUSBDevice. Sukrit Bhatnagar (41): util: error: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC util: buffer: typedef 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: modify virCgroupFree to take virCgroupPtr 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: usb: modify virUSBDeviceListAdd to take double pointer 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: netdevvlan: define cleanup function using VIR_DEFINE_AUTOPTR_FUNC 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 | 38 ++- src/util/virbuffer.h | 9 +- src/util/vircgroup.c | 746 +++++++++++++++++------------------------------ 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 | 26 +- src/util/virhostdev.c | 168 ++++------- 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 | 352 ++++++++-------------- src/util/virpci.h | 7 + src/util/virscsi.c | 63 ++-- src/util/virscsi.h | 3 + src/util/virscsivhost.c | 19 +- src/util/virscsivhost.h | 3 + src/util/virusb.c | 42 ++- src/util/virusb.h | 5 +- tests/vircgrouptest.c | 42 +-- tests/virusbtest.c | 4 +- 42 files changed, 800 insertions(+), 1294 deletions(-) -- 1.8.3.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. 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> Reviewed-by: Erik Skultety <eskultet@redhat.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
Create and use typedefs virBufferEscapePair and virBufferEscapePairPtr for struct _virBufferEscapePair for cleaner code and for use with cleanup macros. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virbuffer.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index 3d6defb..ea96704 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -648,12 +648,14 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, } +typedef struct _virBufferEscapePair virBufferEscapePair; +typedef virBufferEscapePair *virBufferEscapePairPtr; + struct _virBufferEscapePair { char escape; char *toescape; }; - /** * virBufferEscapeN: * @buf: the buffer to append to @@ -678,8 +680,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
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 | 16 ++++++++++++++-- src/util/virbuffer.h | 9 +++++++-- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index ea96704..0640cfa 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -31,10 +31,10 @@ #define __VIR_BUFFER_C__ #include "virbuffer.h" -#include "viralloc.h" #include "virerror.h" #include "virstring.h" +#define VIR_FROM_THIS VIR_FROM_NONE /* If adding more fields, ensure to edit buf.h to match the number of fields */ @@ -656,6 +656,18 @@ struct _virBufferEscapePair { char *toescape; }; +static void +virBufferEscapePairFree(virBufferEscapePairPtr pair) +{ + if (!pair) + return; + + VIR_FREE(pair->toescape); + VIR_FREE(pair); +} +VIR_DEFINE_AUTOPTR_FUNC(virBufferEscapePair, virBufferEscapePairFree) + + /** * virBufferEscapeN: * @buf: the buffer to append to @@ -696,7 +708,7 @@ virBufferEscapeN(virBufferPtr buf, va_start(ap, str); while ((escapeItem.escape = va_arg(ap, int))) { - if (!(escapeItem.toescape = va_arg(ap, char *))) { + if (VIR_STRDUP(escapeItem.toescape, va_arg(ap, char *)) < 0) { virBufferSetError(buf, errno); goto cleanup; } 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
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> --- 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 0640cfa..f70069e 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -456,7 +456,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, @@ -533,7 +534,6 @@ virBufferEscapeString(virBufferPtr buf, const char *format, const char *str) *out = 0; virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); } /** @@ -612,7 +612,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)) @@ -644,7 +645,6 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, *out = 0; virBufferAsprintf(buf, format, escaped); - VIR_FREE(escaped); } @@ -689,7 +689,7 @@ virBufferEscapeN(virBufferPtr buf, { int len; size_t i; - char *escaped = NULL; + VIR_AUTOFREE(char *) escaped = NULL; char *out; const char *cur; virBufferEscapePair escapeItem; @@ -752,7 +752,6 @@ virBufferEscapeN(virBufferPtr buf, cleanup: va_end(ap); VIR_FREE(escapeList); - VIR_FREE(escaped); } @@ -817,7 +816,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)) @@ -861,7 +861,6 @@ virBufferEscapeShell(virBufferPtr buf, const char *str) *out = 0; virBufferAdd(buf, escaped, -1); - VIR_FREE(escaped); } /** -- 1.8.3.1
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 f70069e..7e66b4d 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -693,7 +693,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; @@ -751,7 +751,6 @@ virBufferEscapeN(virBufferPtr buf, cleanup: va_end(ap); - VIR_FREE(escapeList); } -- 1.8.3.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. 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> --- 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
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 is a step towards having consistent function signatures for Free helpers so that they can be used with VIR_AUTOPTR cleanup macro. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> Reviewed-by: Erik Skultety <eskultet@redhat.com> --- src/libvirt-lxc.c | 4 ++-- src/lxc/lxc_cgroup.c | 4 ++-- src/lxc/lxc_container.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_domain.c | 2 +- src/lxc/lxc_process.c | 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 de05627..bda5381 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 ca04be3..c2d3bd5 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 bba157b..59dc3fe 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
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. 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
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> --- 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..6f7b5b4 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 = NULL; + char *p2 = NULL; 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 = NULL; + char *p2 = NULL; 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
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> --- src/util/vircgroup.c | 185 +++++++++++++++++++-------------------------------- 1 file changed, 67 insertions(+), 118 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6f7b5b4..61fafe2 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; } @@ -1277,10 +1268,10 @@ virCgroupNewPartition(const char *path, int controllers, virCgroupPtr *group) { - int ret = -1; VIR_AUTOFREE(char *) parentPath = NULL; VIR_AUTOFREE(char *) newPath = NULL; - virCgroupPtr parent = NULL; + VIR_AUTOPTR(virCgroup) parent = NULL; + VIR_AUTOPTR(virCgroup) tmpGroup = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers); @@ -1315,12 +1306,11 @@ virCgroupNewPartition(const char *path, } } - ret = 0; + return 0; + cleanup: - if (ret != 0) - virCgroupFree(*group); - virCgroupFree(parent); - return ret; + VIR_STEAL_PTR(tmpGroup, *group); + return -1; } @@ -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, - NULL, - virCgroupPidCode, - virCgroupPidEqual, - virCgroupPidCopy, - NULL); + 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, - NULL, - virCgroupPidCode, - virCgroupPidEqual, - virCgroupPidCopy, - NULL); + 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 Tue, Jul 24, 2018 at 09:22:11PM +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> --- ...
@@ -1277,10 +1268,10 @@ virCgroupNewPartition(const char *path, int controllers, virCgroupPtr *group) { - int ret = -1; VIR_AUTOFREE(char *) parentPath = NULL; VIR_AUTOFREE(char *) newPath = NULL; - virCgroupPtr parent = NULL; + VIR_AUTOPTR(virCgroup) parent = NULL; + VIR_AUTOPTR(virCgroup) tmpGroup = NULL; VIR_DEBUG("path=%s create=%d controllers=%x", path, create, controllers);
@@ -1315,12 +1306,11 @@ virCgroupNewPartition(const char *path, } }
- ret = 0; + return 0; + cleanup: - if (ret != 0) - virCgroupFree(*group); - virCgroupFree(parent); - return ret; + VIR_STEAL_PTR(tmpGroup, *group); + return -1;
^Not exactly what I had in mind, we're still touching the caller provided data too much. Have a look at this diff: diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 61fafe26f8..472a8167f5 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1283,34 +1283,31 @@ virCgroupNewPartition(const char *path, } if (virCgroupSetPartitionSuffix(path, &newPath) < 0) - goto cleanup; + return -1; - if (virCgroupNew(-1, newPath, NULL, controllers, group) < 0) - goto cleanup; + if (virCgroupNew(-1, newPath, NULL, controllers, &tmpGroup) < 0) + return -1; if (STRNEQ(newPath, "/")) { char *tmp; if (VIR_STRDUP(parentPath, newPath) < 0) - goto cleanup; + return -1; tmp = strrchr(parentPath, '/'); tmp++; *tmp = '\0'; if (virCgroupNew(-1, parentPath, NULL, controllers, &parent) < 0) - goto cleanup; + return -1; - if (virCgroupMakeGroup(parent, *group, create, VIR_CGROUP_NONE) < 0) { - virCgroupRemove(*group); - goto cleanup; + if (virCgroupMakeGroup(parent, tmpGroup, create, VIR_CGROUP_NONE) < 0) { + virCgroupRemove(tmpGroup); + return -1; } } + VIR_STEAL_PTR(*group, tmpGroup); return 0; - - cleanup: - VIR_STEAL_PTR(tmpGroup, *group); - return -1; After ^this, we'll only touch the caller provided data once we're sure nothing can go wrong anymore. I'll squash that in. 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 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> --- 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
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> --- 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..81fb847 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 NULL; - 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
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> --- 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 81fb847..4050835 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 -1; \ } \ } 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
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> --- 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
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
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> --- 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
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> --- src/util/virhook.c | 22 +++++++++------------- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/src/util/virhook.c b/src/util/virhook.c index facd74a..4673655 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,17 @@ virHookCheck(int no, const char *driver) } if (!virFileExists(path)) { - ret = 0; VIR_DEBUG("No hook script %s", path); - } else if (!virFileIsExecutable(path)) { - ret = 0; + return 0; + } + + if (!virFileIsExecutable(path)) { VIR_WARN("Non-executable hook script %s", path); - } else { - ret = 1; - VIR_DEBUG("Found hook script %s", path); + return 0; } - VIR_FREE(path); - return ret; + VIR_DEBUG("Found hook script %s", path); + return 1; } /* @@ -233,7 +231,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 +316,5 @@ virHookCall(int driver, virCommandFree(cmd); - VIR_FREE(path); - return ret; } -- 1.8.3.1
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> --- 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 4673655..993f06d 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -232,7 +232,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; @@ -314,7 +314,5 @@ virHookCall(int driver, virGetLastErrorMessage()); } - virCommandFree(cmd); - return ret; } -- 1.8.3.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. When variables of types virPCIDevicePtr, virPCIDeviceAddressPtr and virPCIEDeviceInfoPtr are declared using VIR_AUTOPTR, the functions virPCIDeviceFree, virPCIDeviceAddressFree 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> --- src/util/virpci.c | 7 ++++++- src/util/virpci.h | 7 +++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 8d02366..46f9905 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" @@ -3288,3 +3287,9 @@ virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev) VIR_FREE(dev->link_sta); VIR_FREE(dev); } + +void +virPCIDeviceAddressFree(virPCIDeviceAddressPtr address) +{ + VIR_FREE(address); +} diff --git a/src/util/virpci.h b/src/util/virpci.h index 794b7e5..2ac8769 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,10 @@ void virPCIEDeviceInfoFree(virPCIEDeviceInfoPtr dev); ssize_t virPCIGetMdevTypes(const char *sysfspath, virMediatedDeviceType ***types); +void virPCIDeviceAddressFree(virPCIDeviceAddressPtr address); + +VIR_DEFINE_AUTOPTR_FUNC(virPCIDevice, virPCIDeviceFree) +VIR_DEFINE_AUTOPTR_FUNC(virPCIDeviceAddress, virPCIDeviceAddressFree) +VIR_DEFINE_AUTOPTR_FUNC(virPCIEDeviceInfo, virPCIEDeviceInfoFree) + #endif /* __VIR_PCI_H__ */ -- 1.8.3.1
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 | 269 ++++++++++++++++++------------------------------------ 1 file changed, 87 insertions(+), 182 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 46f9905..1b3e52f 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); - else - ret = virPCIDeviceUnbindFromStubWithNewid(dev); + return virPCIDeviceUnbindFromStubWithOverride(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); - else - ret = virPCIDeviceBindToStubWithNewid(dev); + return virPCIDeviceBindToStubWithOverride(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; @@ -1806,14 +1752,12 @@ 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; + goto cleanup;; } @@ -2129,8 +2073,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 +2088,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 +2103,6 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, goto cleanup; if ((actor)(dev, file, opaque) < 0) goto cleanup; - - VIR_FREE(file); } } if (direrr < 0) @@ -2170,8 +2112,6 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, cleanup: VIR_DIR_CLOSE(dir); - VIR_FREE(file); - VIR_FREE(pcidir); return ret; } @@ -2186,7 +2126,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 +2162,6 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, ret = 0; cleanup: - VIR_FREE(groupPath); VIR_DIR_CLOSE(groupDir); return ret; } @@ -2341,28 +2280,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 +2307,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 +2320,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 +2542,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 +2559,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 +2590,7 @@ int virPCIGetPhysicalFunction(const char *vf_sysfs_path, virPCIDeviceAddressPtr *pf) { - char *device_link = NULL; + VIR_AUTOFREE(char *) device_link = NULL; *pf = NULL; @@ -2678,7 +2603,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 +2619,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 +2643,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 +2661,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 +2685,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 +2783,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 +2861,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 +2871,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 +2928,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 +2941,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 +2959,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 +2969,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 +2982,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
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 | 78 +++++++++++++++++++++---------------------------------- 1 file changed, 29 insertions(+), 49 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index 1b3e52f..634df8d 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,13 @@ 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; - } + if (!dev || VIR_STRDUP(*pciConfigAddr, dev->name) < 0) + return -1; - cleanup: - virPCIDeviceFree(dev); - return ret; + return 0; } virPCIDevicePtr @@ -1700,7 +1690,8 @@ virPCIDeviceNew(unsigned int domain, unsigned int slot, unsigned int function) { - virPCIDevicePtr dev; + virPCIDevicePtr ret = NULL; + VIR_AUTOPTR(virPCIDevice) dev = NULL; VIR_AUTOFREE(char *) vendor = NULL; VIR_AUTOFREE(char *) product = NULL; @@ -1717,17 +1708,17 @@ virPCIDeviceNew(unsigned int domain, virReportError(VIR_ERR_INTERNAL_ERROR, _("dev->name buffer overflow: %.4x:%.2x:%.2x.%.1x"), domain, bus, slot, function); - goto error; + goto cleanup; } if (virAsprintf(&dev->path, PCI_SYSFS "devices/%s/config", dev->name) < 0) - goto error; + goto cleanup; if (!virFileExists(dev->path)) { virReportSystemError(errno, _("Device %s not found: could not access %s"), dev->name, dev->path); - goto error; + goto cleanup; } vendor = virPCIDeviceReadID(dev, "vendor"); @@ -1737,7 +1728,7 @@ virPCIDeviceNew(unsigned int domain, virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to read product/vendor ID for %s"), dev->name); - goto error; + goto cleanup; } /* strings contain '0x' prefix */ @@ -1746,18 +1737,15 @@ virPCIDeviceNew(unsigned int domain, virReportError(VIR_ERR_INTERNAL_ERROR, _("dev->id buffer overflow: %s %s"), &vendor[2], &product[2]); - goto error; + goto cleanup; } VIR_DEBUG("%s %s: initialized", dev->id, dev->name); - cleanup: - return dev; + VIR_STEAL_PTR(ret, dev); - error: - virPCIDeviceFree(dev); - dev = NULL; - goto cleanup;; + cleanup: + return ret; } @@ -1960,14 +1948,14 @@ virPCIDeviceListAdd(virPCIDeviceListPtr list, int virPCIDeviceListAddCopy(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - virPCIDevicePtr copy = virPCIDeviceCopy(dev); + VIR_AUTOPTR(virPCIDevice) copy = virPCIDeviceCopy(dev); if (!copy) return -1; - if (virPCIDeviceListAdd(list, copy) < 0) { - virPCIDeviceFree(copy); + if (virPCIDeviceListAdd(list, copy) < 0) return -1; - } + + copy = NULL; return 0; } @@ -2015,8 +2003,7 @@ void virPCIDeviceListDel(virPCIDeviceListPtr list, virPCIDevicePtr dev) { - virPCIDevicePtr ret = virPCIDeviceListSteal(list, dev); - virPCIDeviceFree(ret); + VIR_AUTOPTR(virPCIDevice) ret = virPCIDeviceListSteal(list, dev); } int @@ -2170,22 +2157,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; } @@ -2397,7 +2380,7 @@ virPCIDeviceDownstreamLacksACS(virPCIDevicePtr dev) static int virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) { - virPCIDevicePtr parent; + VIR_AUTOPTR(virPCIDevice) parent = NULL; if (virPCIDeviceGetParent(dev, &parent) < 0) return -1; @@ -2421,14 +2404,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 @@ -2437,7 +2419,6 @@ virPCIDeviceIsBehindSwitchLackingACS(virPCIDevicePtr dev) tmp = parent; ret = virPCIDeviceGetParent(parent, &parent); - virPCIDeviceFree(tmp); if (ret < 0) return -1; } while (parent); @@ -2942,7 +2923,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; @@ -2978,7 +2959,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
Modify virUSBDeviceListAdd to take a double pointer to virUSBDevicePtr as the second argument. This will enable usage of cleanup macros upon the virUSBDevicePtr item which is to be added to the list as it will be cleared by virInsertElementsN upon success. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> --- src/util/virhostdev.c | 6 +++--- src/util/virusb.c | 10 +++++----- src/util/virusb.h | 2 +- tests/virusbtest.c | 4 ++-- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/util/virhostdev.c b/src/util/virhostdev.c index f4bd19d..d5075ac 100644 --- a/src/util/virhostdev.c +++ b/src/util/virhostdev.c @@ -1236,7 +1236,7 @@ virHostdevUpdateActiveUSBDevices(virHostdevManagerPtr mgr, virUSBDeviceSetUsedBy(usb, drv_name, dom_name); - if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, usb) < 0) { + if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, &usb) < 0) { virUSBDeviceFree(usb); goto cleanup; } @@ -1406,7 +1406,7 @@ virHostdevMarkUSBDevices(virHostdevManagerPtr mgr, * from the virUSBDeviceList that passed in on success, * perform rollback on failure. */ - if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, usb) < 0) + if (virUSBDeviceListAdd(mgr->activeUSBHostdevs, &usb) < 0) goto error; } @@ -1555,7 +1555,7 @@ virHostdevPrepareUSBDevices(virHostdevManagerPtr mgr, if (virHostdevFindUSBDevice(hostdev, required, &usb) < 0) goto cleanup; - if (usb && virUSBDeviceListAdd(list, usb) < 0) { + if (usb && virUSBDeviceListAdd(list, &usb) < 0) { virUSBDeviceFree(usb); goto cleanup; } diff --git a/src/util/virusb.c b/src/util/virusb.c index 2fe1bfc..7818232 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -181,7 +181,7 @@ virUSBDeviceSearch(unsigned int vendor, if (!usb) goto cleanup; - if (virUSBDeviceListAdd(list, usb) < 0) { + if (virUSBDeviceListAdd(list, &usb) < 0) { virUSBDeviceFree(usb); goto cleanup; } @@ -463,15 +463,15 @@ virUSBDeviceListDispose(void *obj) int virUSBDeviceListAdd(virUSBDeviceListPtr list, - virUSBDevicePtr dev) + virUSBDevicePtr *dev) { - if (virUSBDeviceListFind(list, dev)) { + if (virUSBDeviceListFind(list, *dev)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Device %s is already in use"), - dev->name); + (*dev)->name); return -1; } - return VIR_APPEND_ELEMENT(list->devs, list->count, dev); + return VIR_APPEND_ELEMENT(list->devs, list->count, *dev); } virUSBDevicePtr diff --git a/src/util/virusb.h b/src/util/virusb.h index 716e8c6..078dee6 100644 --- a/src/util/virusb.h +++ b/src/util/virusb.h @@ -88,7 +88,7 @@ int virUSBDeviceFileIterate(virUSBDevicePtr dev, virUSBDeviceListPtr virUSBDeviceListNew(void); int virUSBDeviceListAdd(virUSBDeviceListPtr list, - virUSBDevicePtr dev); + virUSBDevicePtr *dev); virUSBDevicePtr virUSBDeviceListGet(virUSBDeviceListPtr list, int idx); size_t virUSBDeviceListCount(virUSBDeviceListPtr list); diff --git a/tests/virusbtest.c b/tests/virusbtest.c index 8728fe9..05bba2b 100644 --- a/tests/virusbtest.c +++ b/tests/virusbtest.c @@ -173,7 +173,7 @@ testUSBList(const void *opaque ATTRIBUTE_UNUSED) dev = virUSBDeviceListGet(devlist, 0); dev = virUSBDeviceListSteal(devlist, dev); - if (virUSBDeviceListAdd(list, dev) < 0) + if (virUSBDeviceListAdd(list, &dev) < 0) goto cleanup; dev = NULL; } @@ -196,7 +196,7 @@ testUSBList(const void *opaque ATTRIBUTE_UNUSED) dev = virUSBDeviceListGet(devlist, 0); dev = virUSBDeviceListSteal(devlist, dev); - if (virUSBDeviceListAdd(list, dev) < 0) + if (virUSBDeviceListAdd(list, &dev) < 0) goto cleanup; dev = NULL; } -- 1.8.3.1
On Tue, Jul 24, 2018 at 09:22:23PM +0530, Sukrit Bhatnagar wrote:
Modify virUSBDeviceListAdd to take a double pointer to virUSBDevicePtr as the second argument. This will enable usage of cleanup macros upon the virUSBDevicePtr item which is to be added to the list as it will be cleared by virInsertElementsN upon success.
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 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> --- 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 7818232..90f947b 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 078dee6..afaaf95 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
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> --- 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 90f947b..47b407b 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
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 | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/util/virusb.c b/src/util/virusb.c index 47b407b..df94959 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -123,8 +123,10 @@ virUSBDeviceSearch(unsigned int vendor, bool found = false; char *ignore = NULL; struct dirent *de; - virUSBDeviceListPtr list = NULL, ret = NULL; - virUSBDevicePtr usb; + virUSBDeviceListPtr list = NULL; + virUSBDeviceListPtr ret = NULL; + VIR_AUTOPTR(virUSBDevice) usb = NULL; + virUSBDevicePtr *ptr = NULL; int direrr; if (!(list = virUSBDeviceListNew())) @@ -173,13 +175,13 @@ virUSBDeviceSearch(unsigned int vendor, } usb = virUSBDeviceNew(found_bus, found_devno, vroot); + ptr = &usb; + if (!usb) goto cleanup; - if (virUSBDeviceListAdd(list, &usb) < 0) { - virUSBDeviceFree(usb); + if (virUSBDeviceListAdd(list, ptr) < 0) goto cleanup; - } if (found) break; @@ -508,8 +510,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 Tue, Jul 24, 2018 at 09:22:26PM +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 | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/src/util/virusb.c b/src/util/virusb.c index 47b407b..df94959 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -123,8 +123,10 @@ virUSBDeviceSearch(unsigned int vendor, bool found = false; char *ignore = NULL; struct dirent *de; - virUSBDeviceListPtr list = NULL, ret = NULL; - virUSBDevicePtr usb; + virUSBDeviceListPtr list = NULL; + virUSBDeviceListPtr ret = NULL; + VIR_AUTOPTR(virUSBDevice) usb = NULL; + virUSBDevicePtr *ptr = NULL; int direrr;
if (!(list = virUSBDeviceListNew())) @@ -173,13 +175,13 @@ virUSBDeviceSearch(unsigned int vendor, }
usb = virUSBDeviceNew(found_bus, found_devno, vroot); + ptr = &usb;
I don't see a reason for the @ptr variable, you can work with @usb directly, I'll make the change before pushing. Reviewed-by: Erik Skultety <eskultet@redhat.com>
+ if (!usb) goto cleanup;
- if (virUSBDeviceListAdd(list, &usb) < 0) { - virUSBDeviceFree(usb); + if (virUSBDeviceListAdd(list, ptr) < 0) goto cleanup; - }
if (found) break; @@ -508,8 +510,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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 virUsedByInfo type for use with the cleanup macros. Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com> Reviewed-by: Erik Skultety <eskultet@redhat.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
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> --- 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
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> --- src/util/virscsi.c | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-) diff --git a/src/util/virscsi.c b/src/util/virscsi.c index ba0a688..f97f6b5 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,43 @@ 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); + VIR_STEAL_PTR(ret, dev); return ret; } @@ -281,21 +279,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 +436,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 +445,8 @@ virSCSIDeviceListDel(virSCSIDeviceListPtr list, virSCSIDeviceUsedByInfoFree(dev->used_by[i]); VIR_DELETE_ELEMENT(dev->used_by, i, dev->n_used_by); } else { + VIR_AUTOPTR(virSCSIDevice) tmp = NULL; tmp = virSCSIDeviceListSteal(list, dev); - virSCSIDeviceFree(tmp); } break; } -- 1.8.3.1
On Tue, Jul 24, 2018 at 09:22:29PM +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> --- src/util/virscsi.c | 37 +++++++++++++++---------------------- 1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/src/util/virscsi.c b/src/util/virscsi.c index ba0a688..f97f6b5 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,43 @@ 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); + VIR_STEAL_PTR(ret, dev); return ret; }
@@ -281,21 +279,17 @@ virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *drvname, const char *domname) { - virUsedByInfoPtr copy; + VIR_AUTOPTR(virUsedByInfo) copy = NULL;
I'll add a new line ^here before merging. 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 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> --- 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
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> --- src/util/virscsivhost.c | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/util/virscsivhost.c b/src/util/virscsivhost.c index ef216b3..280d0dc 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); } @@ -253,7 +252,8 @@ virSCSIVHostDeviceGetPath(virSCSIVHostDevicePtr dev) virSCSIVHostDevicePtr virSCSIVHostDeviceNew(const char *name) { - virSCSIVHostDevicePtr dev; + VIR_AUTOPTR(virSCSIVHostDevice) dev = NULL; + virSCSIVHostDevicePtr ret = NULL; if (VIR_ALLOC(dev) < 0) return NULL; @@ -262,22 +262,18 @@ virSCSIVHostDeviceNew(const char *name) virReportError(VIR_ERR_INTERNAL_ERROR, _("dev->name buffer overflow: %s"), name); - goto error; + return NULL; } if (virAsprintf(&dev->path, "%s/%s", SYSFS_VHOST_SCSI_DEVICES, name) < 0) - goto error; + return NULL; VIR_DEBUG("%s: initialized", dev->name); - cleanup: - return dev; + VIR_STEAL_PTR(ret, dev); - error: - virSCSIVHostDeviceFree(dev); - dev = NULL; - goto cleanup; + return ret; } -- 1.8.3.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. 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> Reviewed-by: Erik Skultety <eskultet@redhat.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
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> --- 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 d5075ac..492c42f 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
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> --- 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 492c42f..ca79c37 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
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> --- 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
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> --- 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
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> --- 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
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> --- 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..a55e062 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 NULL; 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 Tue, Jul 24, 2018 at 09:22: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> Reviewed-by: Erik Skultety <eskultet@redhat.com>
For the time being, I'm going to retract my ^RB (the same goes for the previous patch). There are some merge conflicts while applying this and the previous patch (there's been a work on this module recently), so you should resolve them a sent as part of the next batch. Erik
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> --- 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
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> --- 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
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> --- 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
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> --- 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 Tue, Jul 24, 2018 at 09:22:01PM +0530, Sukrit Bhatnagar wrote:
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 and that of virUSBDeviceListAdd is changed to take a double pointer to virUSBDevice.
So I tweaked a few patches as noted in my review and pushed the series. I left out the iscsi patches as there were some merge conflicts that need to be resolved. Thanks, Erik
participants (2)
-
Erik Skultety -
Sukrit Bhatnagar