[PATCH 00/12] use g_autoptr for all DIR*
I don't even remember why I started looking at this. I think that again I was considering changing some function, and making the DIR* in that function autoclose was a prerequisite for a reasonably clean addition to the function. I eventually gave up on the other plan (probably because it was a bad idea), but decided that the DIR*'s really did need to autoclose. In the end, all uses of DIR* could be easily converted to use g_autoptr. Laine Stump (12): consistently use VIR_DIR_CLOSE() instead of virDirClose() tools: reduce scope of a DIR* in virHostValidateIOMMU() storage: remove extraneous call to VIR_DIR_CLOSE() util: reduce scope of a DIR * in virCgroupV1SetOwner() util: manually set dirp to NULL after closing in virCapabilitiesInitCache() util: change virDirClose to take a DIR* instead of DIR**. util: declare g_autoptr cleanup function to auto-close DIR* change DIR* int g_autoptr(DIR) where appropriate conf: convert final DIR* to g_autoptr util: remove unused VIR_DIR_CLOSE() macro util: refactor function to simplify and remove label remove unnecessary cleanup labels and unused return variables src/bhyve/bhyve_capabilities.c | 3 +- src/conf/capabilities.c | 5 +- src/conf/virdomainobjlist.c | 3 +- src/conf/virnetworkobj.c | 44 +++++--------- src/conf/virnwfilterbindingobjlist.c | 3 +- src/conf/virnwfilterobj.c | 3 +- src/conf/virsecretobj.c | 3 +- src/conf/virstorageobj.c | 6 +- src/openvz/openvz_conf.c | 3 +- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_interop_config.c | 14 ++--- src/security/security_selinux.c | 8 +-- src/storage/storage_backend_iscsi.c | 3 +- src/storage/storage_util.c | 69 ++++++++------------- src/util/vircgroup.c | 23 +++---- src/util/vircgroupv1.c | 5 +- src/util/vircommand.c | 12 ++-- src/util/virdevmapper.c | 9 +-- src/util/virfile.c | 65 ++++++++------------ src/util/virfile.h | 4 +- src/util/virhook.c | 8 +-- src/util/virhostcpu.c | 6 +- src/util/virnetdev.c | 13 ++-- src/util/virnuma.c | 24 +++----- src/util/virpci.c | 91 +++++++++++----------------- src/util/virprocess.c | 3 +- src/util/virresctrl.c | 30 ++++----- src/util/virscsi.c | 32 ++++------ src/util/virscsihost.c | 3 +- src/util/virusb.c | 3 +- src/util/virutil.c | 26 +++----- src/util/virvhba.c | 20 +++--- tests/testutilsqemu.c | 35 ++++------- tests/virschematest.c | 3 +- tools/virt-host-validate-common.c | 4 +- 35 files changed, 206 insertions(+), 386 deletions(-) -- 2.26.2
This will make it easier to review upcoming patches that use g_autoptr to auto-close all DIRs. Signed-off-by: Laine Stump <laine@redhat.com> --- src/qemu/qemu_interop_config.c | 2 +- src/security/security_selinux.c | 6 ++---- src/util/virdevmapper.c | 2 +- src/util/virfile.c | 3 +-- tests/testutilsqemu.c | 5 ++--- 5 files changed, 7 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c index 53b251f056..7edca7540a 100644 --- a/src/qemu/qemu_interop_config.c +++ b/src/qemu/qemu_interop_config.c @@ -79,7 +79,7 @@ qemuBuildFileList(virHashTablePtr files, const char *dir) ret = 0; cleanup: - virDirClose(&dirp); + VIR_DIR_CLOSE(dirp); return ret; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 733bcf23d9..7d9e62a239 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3487,8 +3487,7 @@ virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr, virReportSystemError(errno, _("Unable to label files under %s"), path); - virDirClose(&dir); - + VIR_DIR_CLOSE(dir); return ret; } @@ -3532,8 +3531,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, virReportSystemError(errno, _("Unable to restore file labels under %s"), path); - virDirClose(&dir); - + VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 4d27c9f104..f26d36832a 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -211,7 +211,7 @@ virDMSanitizepath(const char *path) } } - virDirClose(&dh); + VIR_DIR_CLOSE(dh); return ret; } diff --git a/src/util/virfile.c b/src/util/virfile.c index 7d0a40b0fb..970d4bd234 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2983,8 +2983,7 @@ int virFileChownFiles(const char *name, ret = 0; cleanup: - virDirClose(&dir); - + VIR_DIR_CLOSE(dir); return ret; } diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 4defba0b7b..278587767f 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -558,7 +558,7 @@ testQemuGetLatestCapsForArch(const char *arch, ret = g_strdup_printf("%s/%s", TEST_QEMU_CAPS_PATH, maxname); cleanup: - virDirClose(&dir); + VIR_DIR_CLOSE(dir); return ret; } @@ -667,8 +667,7 @@ testQemuCapsIterate(const char *suffix, ret = 0; cleanup: - virDirClose(&dir); - + VIR_DIR_CLOSE(dir); return ret; } -- 2.26.2
This will make the trivial nature of a conversion to g_autoptr (in a later patch) more obvious. Signed-off-by: Laine Stump <laine@redhat.com> --- tools/virt-host-validate-common.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 9779eb7b3b..61284ae4da 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -336,7 +336,6 @@ int virHostValidateIOMMU(const char *hvname, bool isAMD = false, isIntel = false; virArch arch = virArchFromHost(); struct dirent *dent; - DIR *dir; int rc; flags = virHostValidateGetCPUFlags(); @@ -375,6 +374,8 @@ int virHostValidateIOMMU(const char *hvname, } else if (ARCH_IS_PPC64(arch)) { /* Empty Block */ } else if (ARCH_IS_S390(arch)) { + DIR *dir; + /* On s390x, we skip the IOMMU check if there are no PCI * devices (which is quite usual on s390x). If there are * no PCI devices the directory is still there but is -- 2.26.2
VIR_DIR_CLOSE(dir) is called in the middle of virStorageBackendRefreshLocal(), which is okay, but redundant - there is no reference to dir between that call and the end of the function, where VIR_DIR_CLOSE() is called again. Remove the extra call in the middle to simplify the function and make the conversion to g_autoptr trivial/mechanical. Signed-off-by: Laine Stump <laine@redhat.com> --- src/storage/storage_util.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 21ad54ac54..7eaf899883 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3553,7 +3553,6 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) } if (direrr < 0) goto cleanup; - VIR_DIR_CLOSE(dir); target = virStorageSourceNew(); -- 2.26.2
DIR *dh is being re-used each time through the for loop of this function, so it must be closed and then re-opened, which means we can't convert it to g_autoptr. By moving the definition of dh inside the for loop, we make it possible to trivially convert to g_autoptr (which will happen in a subsequent patch) NB: VIR_DIR_CLOSE() is already called at the bottom of the for loop, so removing the VIR_DIR_CLOSE() at the end of the function is *not* creating a leak of a DIR*! Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/vircgroupv1.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 91c1c4d4b1..67b35c1b9a 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -875,12 +875,12 @@ virCgroupV1SetOwner(virCgroupPtr cgroup, { int ret = -1; size_t i; - DIR *dh = NULL; int direrr; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { g_autofree char *base = NULL; struct dirent *de; + DIR *dh = NULL; if (!((1 << i) & controllers)) continue; @@ -922,7 +922,6 @@ virCgroupV1SetOwner(virCgroupPtr cgroup, ret = 0; cleanup: - VIR_DIR_CLOSE(dh); return ret; } -- 2.26.2
In all uses of VIR_DIR_CLOSE() except one, the DIR* is never referenced after closing all the way until it goes out of scope. virCapabilitiesInitCaches(), however, reuses the same DIR* over and over in a loop, but due to having many error conditions that result in a goto out of the loop, it's not well suited to reducing the scope of the variable until we introduce a g_autoptr cleanup function for DIR*. In preparation for doing just that, we need to get rid of the side effect of VIR_DIR_CLOSE() setting the DIR* to NULL, so in this one case, let's manually set the DIR* to NULL. Then in an upcoming patch we can safely remove the side effect from VIR_DIR_CLOSE(). This extra/ugly bit of code is only temporary: once we introduce the g_autoptr cleanup function for DIR*, we will remove this manual close/clear completely anyway. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/capabilities.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 90ad4e0c13..18b2612d2e 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1865,6 +1865,7 @@ virCapabilitiesInitCaches(virCapsPtr caps) path = g_strdup_printf("%s/cpu/cpu%zd/cache/", SYSFS_SYSTEM_PATH, pos); VIR_DIR_CLOSE(dirp); + dirp = NULL; rv = virDirOpenIfExists(&dirp, path); if (rv < 0) -- 2.26.2
In order to make a usable g_autoptr(DIR), we need to have a close function that is a NOP when the pointer is NULL, but takes a simple DIR*. But virDirClose() (candidate to be the g_autoptr cleanup function) currently takes a DIR**, not DIR*. It does this so that it can clear the pointer, thus making it safe to call virDirClose on the same DIR multiple times. In the past the clearing of the DIR* was essential in a few places, but those few places have now been changed, so we can modify virDirClose() to take a DIR*, and remove the side effect of clearing the DIR*. This will make it directly usable as the g_autoptr cleanup, and will mean that this: { DIR *dirp = NULL; blah blah ... VIR_DIR_CLOSE(dirp) } is functionally identical to { g_autoptr(DIR) dirp = NULL; blah blah ... } which will make conversion to using g_autoptr mechanical and simple to review. (Note that virDirClose() will still check for NULL before attempting to close, so that it can always be safely called, as long as the DIR* was initialized to NULL (another prerequisite of becoming a g_autoptr cleanup function) Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfile.c | 7 +++---- src/util/virfile.h | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 970d4bd234..442d2fab96 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2927,13 +2927,12 @@ int virDirRead(DIR *dirp, struct dirent **ent, const char *name) return !!*ent; } -void virDirClose(DIR **dirp) +void virDirClose(DIR *dirp) { - if (!*dirp) + if (!dirp) return; - closedir(*dirp); /* exempt from syntax-check */ - *dirp = NULL; + closedir(dirp); /* exempt from syntax-check */ } diff --git a/src/util/virfile.h b/src/util/virfile.h index 09488398c5..6fde4f88ca 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -269,9 +269,9 @@ int virDirOpenQuiet(DIR **dirp, const char *dirname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; -void virDirClose(DIR **dirp) +void virDirClose(DIR *dirp) ATTRIBUTE_NONNULL(1); -#define VIR_DIR_CLOSE(dir) virDirClose(&(dir)) +#define VIR_DIR_CLOSE(dir) virDirClose(dir) int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT; int virFileMakePathWithMode(const char *path, -- 2.26.2
Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfile.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/util/virfile.h b/src/util/virfile.h index 6fde4f88ca..6973fbd54a 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -271,6 +271,7 @@ int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT; void virDirClose(DIR *dirp) ATTRIBUTE_NONNULL(1); +G_DEFINE_AUTOPTR_CLEANUP_FUNC(DIR, virDirClose); #define VIR_DIR_CLOSE(dir) virDirClose(dir) int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT; -- 2.26.2
All of these conversions are trivial - VIR_DIR_CLOSE() (aka virDirClose()) is called only once on the DIR*, and it happens just before going out of scope. Signed-off-by: Laine Stump <laine@redhat.com> --- src/bhyve/bhyve_capabilities.c | 3 +-- src/conf/virdomainobjlist.c | 3 +-- src/conf/virnetworkobj.c | 12 ++++-------- src/conf/virnwfilterbindingobjlist.c | 3 +-- src/conf/virnwfilterobj.c | 3 +-- src/conf/virsecretobj.c | 3 +-- src/conf/virstorageobj.c | 6 ++---- src/openvz/openvz_conf.c | 3 +-- src/qemu/qemu_driver.c | 6 ++---- src/qemu/qemu_interop_config.c | 3 +-- src/security/security_selinux.c | 6 ++---- src/storage/storage_backend_iscsi.c | 3 +-- src/storage/storage_util.c | 18 +++++------------- src/util/vircgroup.c | 7 ++----- src/util/vircgroupv1.c | 4 +--- src/util/vircommand.c | 3 +-- src/util/virdevmapper.c | 3 +-- src/util/virfile.c | 12 ++++-------- src/util/virhook.c | 8 ++------ src/util/virhostcpu.c | 6 ++---- src/util/virnetdev.c | 3 +-- src/util/virnuma.c | 3 +-- src/util/virpci.c | 15 +++++---------- src/util/virprocess.c | 3 +-- src/util/virresctrl.c | 9 +++------ src/util/virscsi.c | 6 ++---- src/util/virscsihost.c | 3 +-- src/util/virusb.c | 3 +-- src/util/virutil.c | 6 ++---- src/util/virvhba.c | 9 +++------ tests/testutilsqemu.c | 6 ++---- tests/virschematest.c | 3 +-- tools/virt-host-validate-common.c | 3 +-- 33 files changed, 60 insertions(+), 127 deletions(-) diff --git a/src/bhyve/bhyve_capabilities.c b/src/bhyve/bhyve_capabilities.c index 8a9acf52b0..e9b4e5176d 100644 --- a/src/bhyve/bhyve_capabilities.c +++ b/src/bhyve/bhyve_capabilities.c @@ -134,7 +134,7 @@ virBhyveDomainCapsBuild(bhyveConnPtr conn, { virDomainCapsPtr caps = NULL; unsigned int bhyve_caps = 0; - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; size_t firmwares_alloc = 0; virBhyveDriverConfigPtr cfg = virBhyveDriverGetConfig(conn); @@ -171,7 +171,6 @@ virBhyveDomainCapsBuild(bhyveConnPtr conn, cleanup: VIR_FREE(firmwares); - VIR_DIR_CLOSE(dir); virObjectUnref(cfg); return caps; } diff --git a/src/conf/virdomainobjlist.c b/src/conf/virdomainobjlist.c index e9a4b271df..9c10090b32 100644 --- a/src/conf/virdomainobjlist.c +++ b/src/conf/virdomainobjlist.c @@ -588,7 +588,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, virDomainLoadConfigNotify notify, void *opaque) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret = -1; int rc; @@ -633,7 +633,6 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, } } - VIR_DIR_CLOSE(dir); virObjectRWUnlock(doms); return ret; } diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index 46205b163c..acf1442f88 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1072,7 +1072,7 @@ virNetworkObjLoadAllState(virNetworkObjListPtr nets, const char *stateDir, virNetworkXMLOptionPtr xmlopt) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret = -1; int rc; @@ -1097,7 +1097,6 @@ virNetworkObjLoadAllState(virNetworkObjListPtr nets, } cleanup: - VIR_DIR_CLOSE(dir); return ret; } @@ -1108,7 +1107,7 @@ virNetworkObjLoadAllConfigs(virNetworkObjListPtr nets, const char *autostartDir, virNetworkXMLOptionPtr xmlopt) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret = -1; int rc; @@ -1132,7 +1131,6 @@ virNetworkObjLoadAllConfigs(virNetworkObjListPtr nets, virNetworkObjEndAPI(&obj); } - VIR_DIR_CLOSE(dir); return ret; } @@ -1707,7 +1705,7 @@ virNetworkObjDeleteAllPorts(virNetworkObjPtr net, const char *stateDir) { g_autofree char *dir = NULL; - DIR *dh = NULL; + g_autoptr(DIR) dh = NULL; struct dirent *de; int rc; int ret = -1; @@ -1738,7 +1736,6 @@ virNetworkObjDeleteAllPorts(virNetworkObjPtr net, ret = 0; cleanup: - VIR_DIR_CLOSE(dh); return ret; } @@ -1863,7 +1860,7 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net, const char *stateDir) { g_autofree char *dir = NULL; - DIR *dh = NULL; + g_autoptr(DIR) dh = NULL; struct dirent *de; int ret = -1; int rc; @@ -1901,6 +1898,5 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net, ret = 0; cleanup: - VIR_DIR_CLOSE(dh); return ret; } diff --git a/src/conf/virnwfilterbindingobjlist.c b/src/conf/virnwfilterbindingobjlist.c index 4cbb62abfa..194348d062 100644 --- a/src/conf/virnwfilterbindingobjlist.c +++ b/src/conf/virnwfilterbindingobjlist.c @@ -300,7 +300,7 @@ int virNWFilterBindingObjListLoadAllConfigs(virNWFilterBindingObjListPtr bindings, const char *configDir) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret = -1; int rc; @@ -330,7 +330,6 @@ virNWFilterBindingObjListLoadAllConfigs(virNWFilterBindingObjListPtr bindings, VIR_ERROR(_("Failed to load config for binding '%s'"), entry->d_name); } - VIR_DIR_CLOSE(dir); virObjectRWUnlock(bindings); return ret; } diff --git a/src/conf/virnwfilterobj.c b/src/conf/virnwfilterobj.c index fce402f907..a6a5aa01c7 100644 --- a/src/conf/virnwfilterobj.c +++ b/src/conf/virnwfilterobj.c @@ -524,7 +524,7 @@ int virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, const char *configDir) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret = -1; int rc; @@ -543,7 +543,6 @@ virNWFilterObjListLoadAllConfigs(virNWFilterObjListPtr nwfilters, virNWFilterObjUnlock(obj); } - VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 146210fbe7..fda3702415 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -896,7 +896,7 @@ int virSecretLoadAllConfigs(virSecretObjListPtr secrets, const char *configDir) { - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *de; int rc; @@ -926,6 +926,5 @@ virSecretLoadAllConfigs(virSecretObjListPtr secrets, virSecretObjEndAPI(&obj); } - VIR_DIR_CLOSE(dir); return 0; } diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 219594582c..4aff62434f 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -1707,7 +1707,7 @@ int virStoragePoolObjLoadAllState(virStoragePoolObjListPtr pools, const char *stateDir) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret = -1; int rc; @@ -1726,7 +1726,6 @@ virStoragePoolObjLoadAllState(virStoragePoolObjListPtr pools, virStoragePoolObjEndAPI(&obj); } - VIR_DIR_CLOSE(dir); return ret; } @@ -1736,7 +1735,7 @@ virStoragePoolObjLoadAllConfigs(virStoragePoolObjListPtr pools, const char *configDir, const char *autostartDir) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret; int rc; @@ -1768,7 +1767,6 @@ virStoragePoolObjLoadAllConfigs(virStoragePoolObjListPtr pools, VIR_FREE(autostartLink); } - VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 8f1740863c..1783dce233 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -1003,7 +1003,7 @@ openvzSetUUID(int vpsid) static int openvzAssignUUIDs(void) { - DIR *dp; + g_autoptr(DIR) dp = NULL; struct dirent *dent; char *conf_dir; int vpsid; @@ -1028,7 +1028,6 @@ static int openvzAssignUUIDs(void) openvzSetUUID(vpsid); } - VIR_DIR_CLOSE(dp); VIR_FREE(conf_dir); return ret; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f1191c1210..cb56fbbfcf 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -357,7 +357,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, { char *baseDir = (char *)data; g_autofree char *snapDir = NULL; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *entry; virDomainSnapshotDefPtr def = NULL; virDomainMomentObjPtr snap = NULL; @@ -457,7 +457,6 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, ret = 0; cleanup: - VIR_DIR_CLOSE(dir); virObjectUnlock(vm); return ret; } @@ -469,7 +468,7 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm, { char *baseDir = (char *)data; g_autofree char *chkDir = NULL; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *entry; virDomainCheckpointDefPtr def = NULL; virDomainMomentObjPtr chk = NULL; @@ -557,7 +556,6 @@ qemuDomainCheckpointLoad(virDomainObjPtr vm, ret = 0; cleanup: - VIR_DIR_CLOSE(dir); virObjectUnlock(vm); return ret; } diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c index 7edca7540a..f2d83eae93 100644 --- a/src/qemu/qemu_interop_config.c +++ b/src/qemu/qemu_interop_config.c @@ -37,7 +37,7 @@ VIR_LOG_INIT("qemu.qemu_configs"); static int qemuBuildFileList(virHashTablePtr files, const char *dir) { - DIR *dirp; + g_autoptr(DIR) dirp = NULL; struct dirent *ent = NULL; int rc; int ret = -1; @@ -79,7 +79,6 @@ qemuBuildFileList(virHashTablePtr files, const char *dir) ret = 0; cleanup: - VIR_DIR_CLOSE(dirp); return ret; } diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 7d9e62a239..6681ce194b 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -3464,7 +3464,7 @@ virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr, int ret = 0; struct dirent *ent; char *filename = NULL; - DIR *dir; + g_autoptr(DIR) dir = NULL; if ((ret = virSecuritySELinuxSetFilecon(mgr, path, seclabel->imagelabel, true))) return ret; @@ -3487,7 +3487,6 @@ virSecuritySELinuxSetFileLabels(virSecurityManagerPtr mgr, virReportSystemError(errno, _("Unable to label files under %s"), path); - VIR_DIR_CLOSE(dir); return ret; } @@ -3509,7 +3508,7 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, int ret = 0; struct dirent *ent; char *filename = NULL; - DIR *dir; + g_autoptr(DIR) dir = NULL; if ((ret = virSecuritySELinuxRestoreFileLabel(mgr, path, true))) return ret; @@ -3531,7 +3530,6 @@ virSecuritySELinuxRestoreFileLabels(virSecurityManagerPtr mgr, virReportSystemError(errno, _("Unable to restore file labels under %s"), path); - VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 436d5e09c7..45167e4490 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -90,7 +90,7 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path, uint32_t *host) { int ret = -1; - DIR *sysdir = NULL; + g_autoptr(DIR) sysdir = NULL; struct dirent *dirent = NULL; int direrr; @@ -122,7 +122,6 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path, } cleanup: - VIR_DIR_CLOSE(sysdir); return ret; } diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 7eaf899883..5a5e517b15 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1920,7 +1920,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, bool loop) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - DIR *dh; + g_autoptr(DIR) dh = NULL; struct dirent *dent; char *stablepath; int opentries = 0; @@ -1963,7 +1963,6 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, stablepath = g_strdup_printf("%s/%s", def->target.path, dent->d_name); if (virFileLinkPointsTo(stablepath, devpath)) { - VIR_DIR_CLOSE(dh); return stablepath; } @@ -1975,8 +1974,6 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, goto retry; } - VIR_DIR_CLOSE(dh); - ret_strdup: /* Couldn't find any matching stable link so give back * the original non-stable dev path @@ -3505,7 +3502,7 @@ int virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) { virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool); - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *ent; struct statvfs sb; struct stat statbuf; @@ -3595,7 +3592,6 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) ret = 0; cleanup: - VIR_DIR_CLOSE(dir); return ret; } @@ -3726,7 +3722,7 @@ getNewStyleBlockDevice(const char *lun_path, const char *block_name G_GNUC_UNUSED, char **block_device) { - DIR *block_dir = NULL; + g_autoptr(DIR) block_dir = NULL; struct dirent *block_dirent = NULL; int retval = -1; int direrr; @@ -3753,7 +3749,6 @@ getNewStyleBlockDevice(const char *lun_path, retval = 0; cleanup: - VIR_DIR_CLOSE(block_dir); return retval; } @@ -3799,7 +3794,7 @@ getBlockDevice(uint32_t host, uint32_t lun, char **block_device) { - DIR *lun_dir = NULL; + g_autoptr(DIR) lun_dir = NULL; struct dirent *lun_dirent = NULL; int retval = -1; int direrr; @@ -3839,7 +3834,6 @@ getBlockDevice(uint32_t host, retval = 0; cleanup: - VIR_DIR_CLOSE(lun_dir); return retval; } @@ -3968,7 +3962,7 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, int retval = 0; uint32_t bus, target, lun; const char *device_path = "/sys/bus/scsi/devices"; - DIR *devicedir = NULL; + g_autoptr(DIR) devicedir = NULL; struct dirent *lun_dirent = NULL; char devicepattern[64]; int found = 0; @@ -4001,8 +3995,6 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, found++; } - VIR_DIR_CLOSE(devicedir); - if (retval < 0) return -1; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index b1caf11f47..6ba030bf9b 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2303,7 +2303,7 @@ virCgroupGetCpuacctPercpuUsage(virCgroupPtr group, char **usage) int virCgroupRemoveRecursively(char *grppath) { - DIR *grpdir; + g_autoptr(DIR) grpdir = NULL; struct dirent *ent; int rc = 0; int direrr; @@ -2334,8 +2334,6 @@ virCgroupRemoveRecursively(char *grppath) VIR_ERROR(_("Failed to readdir for %s (%d)"), grppath, errno); } - VIR_DIR_CLOSE(grpdir); - VIR_DEBUG("Removing cgroup %s", grppath); if (rmdir(grppath) != 0 && errno != ENOENT) { rc = -errno; @@ -2471,7 +2469,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, int rc; bool killedAny = false; g_autofree char *keypath = NULL; - DIR *dp = NULL; + g_autoptr(DIR) dp = NULL; struct dirent *ent; int direrr; VIR_DEBUG("group=%p path=%s signum=%d pids=%p", @@ -2524,7 +2522,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, ret = killedAny ? 1 : 0; cleanup: - VIR_DIR_CLOSE(dp); return ret; } diff --git a/src/util/vircgroupv1.c b/src/util/vircgroupv1.c index 67b35c1b9a..61fb359f8d 100644 --- a/src/util/vircgroupv1.c +++ b/src/util/vircgroupv1.c @@ -880,7 +880,7 @@ virCgroupV1SetOwner(virCgroupPtr cgroup, for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { g_autofree char *base = NULL; struct dirent *de; - DIR *dh = NULL; + g_autoptr(DIR) dh = NULL; if (!((1 << i) & controllers)) continue; @@ -915,8 +915,6 @@ virCgroupV1SetOwner(virCgroupPtr cgroup, base, uid, gid); goto cleanup; } - - VIR_DIR_CLOSE(dh); } ret = 0; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 6350e77523..5ae2ad9ef9 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -452,7 +452,7 @@ static int virCommandMassCloseGetFDsLinux(virCommandPtr cmd G_GNUC_UNUSED, virBitmapPtr fds) { - DIR *dp = NULL; + g_autoptr(DIR) dp = NULL; struct dirent *entry; const char *dirName = "/proc/self/fd"; int rc; @@ -479,7 +479,6 @@ virCommandMassCloseGetFDsLinux(virCommandPtr cmd G_GNUC_UNUSED, ret = 0; cleanup: - VIR_DIR_CLOSE(dp); return ret; } diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index f26d36832a..323102d36e 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -169,7 +169,7 @@ virDMSanitizepath(const char *path) g_autofree char *dmDirPath = NULL; struct dirent *ent = NULL; struct stat sb[2]; - DIR *dh = NULL; + g_autoptr(DIR) dh = NULL; const char *p; char *ret = NULL; @@ -211,7 +211,6 @@ virDMSanitizepath(const char *path) } } - VIR_DIR_CLOSE(dh); return ret; } diff --git a/src/util/virfile.c b/src/util/virfile.c index 442d2fab96..fc0021036a 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -669,7 +669,7 @@ static int virFileLoopDeviceOpenLoopCtl(char **dev_name, int *fd) static int virFileLoopDeviceOpenSearch(char **dev_name) { int fd = -1; - DIR *dh = NULL; + g_autoptr(DIR) dh = NULL; struct dirent *de; char *looppath = NULL; struct loop_info64 lo; @@ -726,7 +726,6 @@ static int virFileLoopDeviceOpenSearch(char **dev_name) VIR_DEBUG("No free loop devices available"); VIR_FREE(looppath); } - VIR_DIR_CLOSE(dh); return fd; } @@ -836,7 +835,7 @@ virFileNBDDeviceIsBusy(const char *dev_name) static char * virFileNBDDeviceFindUnused(void) { - DIR *dh; + g_autoptr(DIR) dh = NULL; char *ret = NULL; struct dirent *de; int direrr; @@ -861,7 +860,6 @@ virFileNBDDeviceFindUnused(void) _("No free NBD devices")); cleanup: - VIR_DIR_CLOSE(dh); return ret; } @@ -979,7 +977,7 @@ int virFileNBDDeviceAssociate(const char *file, */ int virFileDeleteTree(const char *dir) { - DIR *dh; + g_autoptr(DIR) dh = NULL; struct dirent *de; int ret = -1; int direrr; @@ -1028,7 +1026,6 @@ int virFileDeleteTree(const char *dir) ret = 0; cleanup: - VIR_DIR_CLOSE(dh); return ret; } @@ -2954,7 +2951,7 @@ int virFileChownFiles(const char *name, struct dirent *ent; int ret = -1; int direrr; - DIR *dir; + g_autoptr(DIR) dir = NULL; if (virDirOpen(&dir, name) < 0) return -1; @@ -2982,7 +2979,6 @@ int virFileChownFiles(const char *name, ret = 0; cleanup: - VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/util/virhook.c b/src/util/virhook.c index 119ad1aae6..aad3e667d3 100644 --- a/src/util/virhook.c +++ b/src/util/virhook.c @@ -143,7 +143,7 @@ static int virHookCheck(int no, const char *driver) { int ret; - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; g_autofree char *dir_path = NULL; @@ -199,8 +199,6 @@ virHookCheck(int no, const char *driver) break; } - VIR_DIR_CLOSE(dir); - return ret; } @@ -341,7 +339,7 @@ virHookCall(int driver, char **output) { int ret, script_ret; - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; g_autofree char *dir_path = NULL; @@ -438,8 +436,6 @@ virHookCall(int driver, virStringListAdd(&entries, entry_path); } - VIR_DIR_CLOSE(dir); - if (ret < 0) return -1; diff --git a/src/util/virhostcpu.c b/src/util/virhostcpu.c index 4ae4dca067..c531d65f86 100644 --- a/src/util/virhostcpu.c +++ b/src/util/virhostcpu.c @@ -308,7 +308,7 @@ virHostCPUParseNode(const char *node, { int ret = -1; int processors = 0; - DIR *cpudir = NULL; + g_autoptr(DIR) cpudir = NULL; struct dirent *cpudirent = NULL; virBitmapPtr node_cpus_map = NULL; virBitmapPtr sockets_map = NULL; @@ -443,7 +443,6 @@ virHostCPUParseNode(const char *node, ret = processors; cleanup: - VIR_DIR_CLOSE(cpudir); if (cores_maps) for (i = 0; i < sock_max; i++) virBitmapFree(cores_maps[i]); @@ -611,7 +610,7 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, { virBitmapPtr present_cpus_map = NULL; virBitmapPtr online_cpus_map = NULL; - DIR *nodedir = NULL; + g_autoptr(DIR) nodedir = NULL; struct dirent *nodedirent = NULL; int nodecpus, nodecores, nodesockets, nodethreads, offline = 0; int threads_per_subcore = 0; @@ -772,7 +771,6 @@ virHostCPUGetInfoPopulateLinux(FILE *cpuinfo, ret = 0; cleanup: - VIR_DIR_CLOSE(nodedir); virBitmapFree(present_cpus_map); virBitmapFree(online_cpus_map); VIR_FREE(sysfs_nodedir); diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 91b12fc131..256179c052 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2898,7 +2898,7 @@ virNetDevRDMAFeature(const char *ifname, { g_autofree char *eth_devpath = NULL; g_autofree char *eth_res_buf = NULL; - DIR *dirp = NULL; + g_autoptr(DIR) dirp = NULL; struct dirent *dp; int ret = -1; @@ -2934,7 +2934,6 @@ virNetDevRDMAFeature(const char *ifname, ret = 0; cleanup: - VIR_DIR_CLOSE(dirp); return ret; } diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 5d40d13977..51a5d6e8a6 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -740,7 +740,7 @@ virNumaGetPages(int node, size_t *npages) { int ret = -1; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; int direrr = 0; struct dirent *entry; unsigned int ntmp = 0; @@ -854,7 +854,6 @@ virNumaGetPages(int node, *npages = ntmp; ret = 0; cleanup: - VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/util/virpci.c b/src/util/virpci.c index 1f679a7b45..b44208e05a 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -432,7 +432,7 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, virPCIDevicePtr *matched, void *data) { - DIR *dir; + g_autoptr(DIR) dir = NULL; struct dirent *entry; int ret = 0; int rc; @@ -480,7 +480,6 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, break; } } - VIR_DIR_CLOSE(dir); return ret; } @@ -1706,7 +1705,7 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, void *opaque) { g_autofree char *pcidir = NULL; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; int ret = -1; struct dirent *ent; int direrr; @@ -1741,7 +1740,6 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, ret = 0; cleanup: - VIR_DIR_CLOSE(dir); return ret; } @@ -1757,7 +1755,7 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, void *opaque) { g_autofree char *groupPath = NULL; - DIR *groupDir = NULL; + g_autoptr(DIR) groupDir = NULL; int ret = -1; struct dirent *ent; int direrr; @@ -1790,7 +1788,6 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, ret = 0; cleanup: - VIR_DIR_CLOSE(groupDir); return ret; } @@ -2388,7 +2385,7 @@ virPCIGetNetName(const char *device_link_sysfs_path, g_autofree char *pcidev_sysfs_net_path = NULL; g_autofree char *firstEntryName = NULL; int ret = -1; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *entry = NULL; size_t i = 0; @@ -2461,7 +2458,6 @@ virPCIGetNetName(const char *device_link_sysfs_path, } } cleanup: - VIR_DIR_CLOSE(dir); return ret; } @@ -2535,7 +2531,7 @@ virPCIGetMdevTypes(const char *sysfspath, { ssize_t ret = -1; int dirret = -1; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *types_path = NULL; g_autoptr(virMediatedDeviceType) mdev_type = NULL; @@ -2575,7 +2571,6 @@ virPCIGetMdevTypes(const char *sysfspath, for (i = 0; i < ntypes; i++) virMediatedDeviceTypeFree(mdev_types[i]); VIR_FREE(mdev_types); - VIR_DIR_CLOSE(dir); return ret; } diff --git a/src/util/virprocess.c b/src/util/virprocess.c index 37413796b3..dccfc2558e 100644 --- a/src/util/virprocess.c +++ b/src/util/virprocess.c @@ -616,7 +616,7 @@ virProcessGetAffinity(pid_t pid G_GNUC_UNUSED) int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids) { int ret = -1; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; int value; struct dirent *ent; g_autofree char *taskPath = NULL; @@ -647,7 +647,6 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids) ret = 0; cleanup: - VIR_DIR_CLOSE(dir); if (ret < 0) VIR_FREE(*pids); return ret; diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 400c8e9981..aa466592fc 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -781,7 +781,7 @@ virResctrlGetMonitorInfo(virResctrlInfoPtr resctrl) static int virResctrlGetInfo(virResctrlInfoPtr resctrl) { - DIR *dirp = NULL; + g_autoptr(DIR) dirp = NULL; int ret = -1; ret = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info"); @@ -802,7 +802,6 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) ret = 0; cleanup: - VIR_DIR_CLOSE(dirp); return ret; } @@ -1900,7 +1899,7 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) virResctrlAllocPtr ret = NULL; virResctrlAllocPtr alloc = NULL; struct dirent *ent = NULL; - DIR *dirp = NULL; + g_autoptr(DIR) dirp = NULL; int rv = -1; if (virResctrlInfoIsEmpty(resctrl)) { @@ -1947,7 +1946,6 @@ virResctrlAllocGetUnused(virResctrlInfoPtr resctrl) cleanup: virObjectUnref(alloc); - VIR_DIR_CLOSE(dirp); return ret; error: @@ -2663,7 +2661,7 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, int ret = -1; size_t i = 0; unsigned long long val = 0; - DIR *dirp = NULL; + g_autoptr(DIR) dirp = NULL; char *datapath = NULL; char *filepath = NULL; struct dirent *ent = NULL; @@ -2747,7 +2745,6 @@ virResctrlMonitorGetStats(virResctrlMonitorPtr monitor, VIR_FREE(datapath); VIR_FREE(filepath); virResctrlMonitorStatsFree(stat); - VIR_DIR_CLOSE(dirp); return ret; } diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 4dcd68d53a..256acc37fa 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -106,7 +106,7 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, unsigned int target, unsigned long long unit) { - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; char *sg = NULL; @@ -129,7 +129,6 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, } cleanup: - VIR_DIR_CLOSE(dir); return sg; } @@ -143,7 +142,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, unsigned int target, unsigned long long unit) { - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; char *name = NULL; @@ -165,7 +164,6 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, } cleanup: - VIR_DIR_CLOSE(dir); return name; } diff --git a/src/util/virscsihost.c b/src/util/virscsihost.c index 7d8e5299b8..c259e63000 100644 --- a/src/util/virscsihost.c +++ b/src/util/virscsihost.c @@ -100,7 +100,7 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, { const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_HOST_PATH; struct dirent *entry = NULL; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; char *host_link = NULL; char *host_path = NULL; char *p = NULL; @@ -157,7 +157,6 @@ virSCSIHostFindByPCI(const char *sysfs_prefix, } cleanup: - VIR_DIR_CLOSE(dir); VIR_FREE(unique_path); VIR_FREE(host_link); VIR_FREE(host_path); diff --git a/src/util/virusb.c b/src/util/virusb.c index 3e7e4281a3..a8afd6e6f0 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -110,7 +110,7 @@ virUSBDeviceSearch(unsigned int vendor, const char *vroot, unsigned int flags) { - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; bool found = false; char *ignore = NULL; struct dirent *de; @@ -180,7 +180,6 @@ virUSBDeviceSearch(unsigned int vendor, ret = list; cleanup: - VIR_DIR_CLOSE(dir); if (!ret) virObjectUnref(list); return ret; diff --git a/src/util/virutil.c b/src/util/virutil.c index 708c3cf9ef..41e92023fc 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1621,7 +1621,7 @@ virMemoryMaxValue(bool capped) bool virHostHasIOMMU(void) { - DIR *iommuDir = NULL; + g_autoptr(DIR) iommuDir = NULL; struct dirent *iommuGroup = NULL; bool ret = false; int direrr; @@ -1638,7 +1638,6 @@ virHostHasIOMMU(void) ret = true; cleanup: - VIR_DIR_CLOSE(iommuDir); return ret; } @@ -1658,7 +1657,7 @@ char * virHostGetDRMRenderNode(void) { char *ret = NULL; - DIR *driDir = NULL; + g_autoptr(DIR) driDir = NULL; const char *driPath = "/dev/dri"; struct dirent *ent = NULL; int dirErr = 0; @@ -1687,7 +1686,6 @@ virHostGetDRMRenderNode(void) ret = g_strdup_printf("%s/%s", driPath, ent->d_name); cleanup: - VIR_DIR_CLOSE(driDir); return ret; } diff --git a/src/util/virvhba.c b/src/util/virvhba.c index df0691a658..471d94d3dd 100644 --- a/src/util/virvhba.c +++ b/src/util/virvhba.c @@ -158,7 +158,7 @@ char * virVHBAFindVportHost(const char *sysfs_prefix) { const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *entry = NULL; char *max_vports = NULL; char *vports = NULL; @@ -220,7 +220,6 @@ virVHBAFindVportHost(const char *sysfs_prefix) } cleanup: - VIR_DIR_CLOSE(dir); VIR_FREE(max_vports); VIR_FREE(vports); return ret; @@ -365,7 +364,7 @@ virVHBAGetHostByWWN(const char *sysfs_prefix, { const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; struct dirent *entry = NULL; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; char *ret = NULL; if (virDirOpen(&dir, prefix) < 0) @@ -393,7 +392,6 @@ virVHBAGetHostByWWN(const char *sysfs_prefix, } cleanup: - VIR_DIR_CLOSE(dir); return ret; } @@ -411,7 +409,7 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix, { const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; struct dirent *entry = NULL; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; char *vport_create_path = NULL; char *ret = NULL; @@ -443,7 +441,6 @@ virVHBAGetHostByFabricWWN(const char *sysfs_prefix, } cleanup: - VIR_DIR_CLOSE(dir); VIR_FREE(vport_create_path); return ret; } diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 278587767f..5ae1d64337 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -510,7 +510,7 @@ testQemuGetLatestCapsForArch(const char *arch, const char *suffix) { struct dirent *ent; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; int rc; g_autofree char *fullsuffix = NULL; unsigned long maxver = 0; @@ -558,7 +558,6 @@ testQemuGetLatestCapsForArch(const char *arch, ret = g_strdup_printf("%s/%s", TEST_QEMU_CAPS_PATH, maxname); cleanup: - VIR_DIR_CLOSE(dir); return ret; } @@ -606,7 +605,7 @@ testQemuCapsIterate(const char *suffix, void *opaque) { struct dirent *ent; - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; int rc; int ret = -1; bool fail = false; @@ -667,7 +666,6 @@ testQemuCapsIterate(const char *suffix, ret = 0; cleanup: - VIR_DIR_CLOSE(dir); return ret; } diff --git a/tests/virschematest.c b/tests/virschematest.c index 0e587c3ff0..9a947f61f2 100644 --- a/tests/virschematest.c +++ b/tests/virschematest.c @@ -86,7 +86,7 @@ testSchemaDir(const char *schema, const char *dir_path, const char *filterstr) { - DIR *dir = NULL; + g_autoptr(DIR) dir = NULL; struct dirent *ent; int ret = 0; int rc; @@ -128,7 +128,6 @@ testSchemaDir(const char *schema, ret = -1; } - VIR_DIR_CLOSE(dir); return ret; } diff --git a/tools/virt-host-validate-common.c b/tools/virt-host-validate-common.c index 61284ae4da..a10ac03293 100644 --- a/tools/virt-host-validate-common.c +++ b/tools/virt-host-validate-common.c @@ -374,7 +374,7 @@ int virHostValidateIOMMU(const char *hvname, } else if (ARCH_IS_PPC64(arch)) { /* Empty Block */ } else if (ARCH_IS_S390(arch)) { - DIR *dir; + g_autoptr(DIR) dir = NULL; /* On s390x, we skip the IOMMU check if there are no PCI * devices (which is quite usual on s390x). If there are @@ -383,7 +383,6 @@ int virHostValidateIOMMU(const char *hvname, if (!virDirOpen(&dir, "/sys/bus/pci/devices")) return 0; rc = virDirRead(dir, &dent, NULL); - VIR_DIR_CLOSE(dir); if (rc <= 0) return 0; } else { -- 2.26.2
This use of DIR* was re-using the same function-scope DIR* each time through a for loop, and due to multiple error gotos in the loop, it needed to have the scope of the DIR* reduced to just the loop at the same time as switching to g_autoptr. That's what this patch does. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/capabilities.c | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 18b2612d2e..425f34113a 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -1837,7 +1837,6 @@ virCapabilitiesInitCaches(virCapsPtr caps) size_t i = 0; virBitmapPtr cpus = NULL; ssize_t pos = -1; - DIR *dirp = NULL; int ret = -1; char *path = NULL; char *type = NULL; @@ -1860,13 +1859,11 @@ virCapabilitiesInitCaches(virCapsPtr caps) while ((pos = virBitmapNextSetBit(cpus, pos)) >= 0) { int rv = -1; + g_autoptr(DIR) dirp = NULL; VIR_FREE(path); path = g_strdup_printf("%s/cpu/cpu%zd/cache/", SYSFS_SYSTEM_PATH, pos); - VIR_DIR_CLOSE(dirp); - dirp = NULL; - rv = virDirOpenIfExists(&dirp, path); if (rv < 0) goto cleanup; @@ -1971,7 +1968,6 @@ virCapabilitiesInitCaches(virCapsPtr caps) cleanup: VIR_FREE(type); VIR_FREE(path); - VIR_DIR_CLOSE(dirp); virCapsHostCacheBankFree(bank); virBitmapFree(cpus); return ret; -- 2.26.2
Since every single use of DIR* was converted to use g_autoptr, this function is not currently needed. Even if someone comes up with a usage for a non-g_autoptr DIR* in the future, they can just use virDirClose(), since there is no longer a semantic difference between the two (VIR_DIR_CLOSE() previously had an extra & on the pointer so that it could be transparently passed as a DIR** to virDirClose(), but that was removed several commits back.) Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virfile.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/util/virfile.h b/src/util/virfile.h index 6973fbd54a..ba31a78e58 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -272,7 +272,6 @@ int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname) void virDirClose(DIR *dirp) ATTRIBUTE_NONNULL(1); G_DEFINE_AUTOPTR_CLEANUP_FUNC(DIR, virDirClose); -#define VIR_DIR_CLOSE(dir) virDirClose(dir) int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT; int virFileMakePathWithMode(const char *path, -- 2.26.2
Once the DIR* in virPCIGetName() was made g_autoptr, the cleanup: label just had a "return ret;", but the rest of the function was more compilcated than it needed to be, doing funky things with the value of ret inside multi-level conditionals and a while loop that might exit early via a break with ret == 0 or exit early via a goto cleanup with ret == -1. It really didn't need to be nearly as complicated. After doing the trivial replacements of "goto cleanup" with appropriate direct returns, it became obvious that: 1) the outermost level of the nested conditional at the end of the function ("if (ret < 0)") was now redundant, since ret is now *always* < 0 by that point (otherwise the function has returned). 2) by switching the sense of the next level of the conditional (making it "if (!physPortID)", the "else" (which is now just "return 0;" becomes the "if", and the new "else" no longer needs to be inside the conditional. 3) the value of firstEntryName can be moved into *netname with g_steal_pointer() Once that is all done, ret is no longer used and can be removed. Signed-off-by: Laine Stump <laine@redhat.com> --- src/util/virpci.c | 49 +++++++++++++++++++---------------------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index b44208e05a..025eb9d91c 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -2384,7 +2384,6 @@ virPCIGetNetName(const char *device_link_sysfs_path, { g_autofree char *pcidev_sysfs_net_path = NULL; g_autofree char *firstEntryName = NULL; - int ret = -1; g_autoptr(DIR) dir = NULL; struct dirent *entry = NULL; size_t i = 0; @@ -2399,8 +2398,7 @@ virPCIGetNetName(const char *device_link_sysfs_path, if (virDirOpenQuiet(&dir, pcidev_sysfs_net_path) < 0) { /* this *isn't* an error - caller needs to check for netname == NULL */ - ret = 0; - goto cleanup; + return 0; } while (virDirRead(dir, &entry, pcidev_sysfs_net_path) > 0) { @@ -2411,7 +2409,7 @@ virPCIGetNetName(const char *device_link_sysfs_path, g_autofree char *thisPhysPortID = NULL; if (virNetDevGetPhysPortID(entry->d_name, &thisPhysPortID) < 0) - goto cleanup; + return -1; /* if this one doesn't match, keep looking */ if (STRNEQ_NULLABLE(physPortID, thisPhysPortID)) { @@ -2431,34 +2429,27 @@ virPCIGetNetName(const char *device_link_sysfs_path, } *netname = g_strdup(entry->d_name); - - ret = 0; - break; + return 0; } - if (ret < 0) { - if (physPortID) { - if (firstEntryName) { - /* we didn't match the provided phys_port_id, but this - * is probably because phys_port_id isn't implemented - * for this NIC driver, so just return the first - * (probably only) netname we found. - */ - *netname = firstEntryName; - firstEntryName = NULL; - ret = 0; - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not find network device with " - "phys_port_id '%s' under PCI device at %s"), - physPortID, device_link_sysfs_path); - } - } else { - ret = 0; /* no netdev at the given index is *not* an error */ - } + if (!physPortID) + return 0; + + if (firstEntryName) { + /* we didn't match the provided phys_port_id, but this + * is probably because phys_port_id isn't implemented + * for this NIC driver, so just return the first + * (probably only) netname we found. + */ + *netname = g_steal_pointer(&firstEntryName); + return 0; } - cleanup: - return ret; + + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Could not find network device with " + "phys_port_id '%s' under PCI device at %s"), + physPortID, device_link_sysfs_path); + return -1; } int -- 2.26.2
After converting all DIR* to g_autoptr(DIR), many cleanup: labels ended up just having "return ret", and every place that set ret would just immediately goto cleanup. Remove the cleanup label and its return, and just return the set value immediately, thus eliminating the need for the return variable itself. Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/virnetworkobj.c | 32 +++++++--------------- src/qemu/qemu_interop_config.c | 11 +++----- src/storage/storage_util.c | 50 +++++++++++++--------------------- src/util/vircgroup.c | 16 ++++------- src/util/vircommand.c | 9 ++---- src/util/virdevmapper.c | 6 ++-- src/util/virfile.c | 45 ++++++++++++------------------ src/util/virnetdev.c | 10 ++----- src/util/virnuma.c | 21 ++++++-------- src/util/virpci.c | 27 ++++++------------ src/util/virresctrl.c | 21 ++++++-------- src/util/virscsi.c | 26 ++++++------------ src/util/virutil.c | 20 ++++---------- src/util/virvhba.c | 11 +++----- tests/testutilsqemu.c | 28 +++++++------------ 15 files changed, 119 insertions(+), 214 deletions(-) diff --git a/src/conf/virnetworkobj.c b/src/conf/virnetworkobj.c index acf1442f88..8b3eb0f41c 100644 --- a/src/conf/virnetworkobj.c +++ b/src/conf/virnetworkobj.c @@ -1091,12 +1091,11 @@ virNetworkObjLoadAllState(virNetworkObjListPtr nets, if (obj && virNetworkObjLoadAllPorts(obj, stateDir) < 0) { virNetworkObjEndAPI(&obj); - goto cleanup; + return -1; } virNetworkObjEndAPI(&obj); } - cleanup: return ret; } @@ -1708,15 +1707,12 @@ virNetworkObjDeleteAllPorts(virNetworkObjPtr net, g_autoptr(DIR) dh = NULL; struct dirent *de; int rc; - int ret = -1; if (!(dir = virNetworkObjGetPortStatusDir(net, stateDir))) - goto cleanup; + return -1; - if ((rc = virDirOpenIfExists(&dh, dir)) <= 0) { - ret = rc; - goto cleanup; - } + if ((rc = virDirOpenIfExists(&dh, dir)) <= 0) + return rc; while ((rc = virDirRead(dh, &de, dir)) > 0) { char *file = NULL; @@ -1733,10 +1729,7 @@ virNetworkObjDeleteAllPorts(virNetworkObjPtr net, } virHashRemoveAll(net->ports); - - ret = 0; - cleanup: - return ret; + return 0; } @@ -1862,18 +1855,15 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net, g_autofree char *dir = NULL; g_autoptr(DIR) dh = NULL; struct dirent *de; - int ret = -1; int rc; char uuidstr[VIR_UUID_STRING_BUFLEN]; g_autoptr(virNetworkPortDef) portdef = NULL; if (!(dir = virNetworkObjGetPortStatusDir(net, stateDir))) - goto cleanup; + return -1; - if ((rc = virDirOpenIfExists(&dh, dir)) <= 0) { - ret = rc; - goto cleanup; - } + if ((rc = virDirOpenIfExists(&dh, dir)) <= 0) + return rc; while ((rc = virDirRead(dh, &de, dir)) > 0) { g_autofree char *file = NULL; @@ -1891,12 +1881,10 @@ virNetworkObjLoadAllPorts(virNetworkObjPtr net, virUUIDFormat(portdef->uuid, uuidstr); if (virHashAddEntry(net->ports, uuidstr, portdef) < 0) - goto cleanup; + return -1; portdef = NULL; } - ret = 0; - cleanup: - return ret; + return 0; } diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c index f2d83eae93..e1c20aff11 100644 --- a/src/qemu/qemu_interop_config.c +++ b/src/qemu/qemu_interop_config.c @@ -40,7 +40,6 @@ qemuBuildFileList(virHashTablePtr files, const char *dir) g_autoptr(DIR) dirp = NULL; struct dirent *ent = NULL; int rc; - int ret = -1; if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) return -1; @@ -62,24 +61,22 @@ qemuBuildFileList(virHashTablePtr files, const char *dir) if (stat(path, &sb) < 0) { virReportSystemError(errno, _("Unable to access %s"), path); - goto cleanup; + return -1; } if (!S_ISREG(sb.st_mode) && !S_ISLNK(sb.st_mode)) continue; if (virHashUpdateEntry(files, filename, path) < 0) - goto cleanup; + return -1; path = NULL; } if (rc < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } static int diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 5a5e517b15..cf1f33f177 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -3507,13 +3507,12 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) struct statvfs sb; struct stat statbuf; int direrr; - int ret = -1; g_autoptr(virStorageVolDef) vol = NULL; VIR_AUTOCLOSE fd = -1; g_autoptr(virStorageSource) target = NULL; if (virDirOpen(&dir, def->target.path) < 0) - goto cleanup; + return -1; while ((direrr = virDirRead(dir, &ent, def->target.path)) > 0) { int err; @@ -3541,15 +3540,15 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) vol = NULL; continue; } - goto cleanup; + return -1; } if (virStoragePoolObjAddVol(pool, vol) < 0) - goto cleanup; + return -1; vol = NULL; } if (direrr < 0) - goto cleanup; + return -1; target = virStorageSourceNew(); @@ -3557,25 +3556,25 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) virReportSystemError(errno, _("cannot open path '%s'"), def->target.path); - goto cleanup; + return -1; } if (fstat(fd, &statbuf) < 0) { virReportSystemError(errno, _("cannot stat path '%s'"), def->target.path); - goto cleanup; + return -1; } if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &statbuf) < 0) - goto cleanup; + return -1; /* VolTargetInfoFD doesn't update capacity correctly for the pool case */ if (statvfs(def->target.path, &sb) < 0) { virReportSystemError(errno, _("cannot statvfs path '%s'"), def->target.path); - goto cleanup; + return -1; } def->capacity = ((unsigned long long)sb.f_frsize * @@ -3590,9 +3589,7 @@ virStorageBackendRefreshLocal(virStoragePoolObjPtr pool) VIR_FREE(def->target.perms.label); def->target.perms.label = g_strdup(target->perms->label); - ret = 0; - cleanup: - return ret; + return 0; } @@ -3724,7 +3721,6 @@ getNewStyleBlockDevice(const char *lun_path, { g_autoptr(DIR) block_dir = NULL; struct dirent *block_dirent = NULL; - int retval = -1; int direrr; g_autofree char *block_path = NULL; @@ -3733,7 +3729,7 @@ getNewStyleBlockDevice(const char *lun_path, VIR_DEBUG("Looking for block device in '%s'", block_path); if (virDirOpen(&block_dir, block_path) < 0) - goto cleanup; + return -1; while ((direrr = virDirRead(block_dir, &block_dirent, block_path)) > 0) { *block_device = g_strdup(block_dirent->d_name); @@ -3744,12 +3740,9 @@ getNewStyleBlockDevice(const char *lun_path, } if (direrr < 0) - goto cleanup; - - retval = 0; + return -1; - cleanup: - return retval; + return 0; } @@ -3796,7 +3789,6 @@ getBlockDevice(uint32_t host, { g_autoptr(DIR) lun_dir = NULL; struct dirent *lun_dirent = NULL; - int retval = -1; int direrr; g_autofree char *lun_path = NULL; @@ -3806,7 +3798,7 @@ getBlockDevice(uint32_t host, target, lun); if (virDirOpen(&lun_dir, lun_path) < 0) - goto cleanup; + return -1; while ((direrr = virDirRead(lun_dir, &lun_dirent, lun_path)) > 0) { if (STRPREFIX(lun_dirent->d_name, "block")) { @@ -3814,27 +3806,23 @@ getBlockDevice(uint32_t host, if (getNewStyleBlockDevice(lun_path, lun_dirent->d_name, block_device) < 0) - goto cleanup; + return -1; } else { if (getOldStyleBlockDevice(lun_path, lun_dirent->d_name, block_device) < 0) - goto cleanup; + return -1; } break; } } if (direrr < 0) - goto cleanup; - if (!*block_device) { - retval = -2; - goto cleanup; - } + return -1; - retval = 0; + if (!*block_device) + return -2; - cleanup: - return retval; + return 0; } diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 6ba030bf9b..e3c45e93cb 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -2465,7 +2465,6 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, const char *taskFile, bool dormdir) { - int ret = -1; int rc; bool killedAny = false; g_autofree char *keypath = NULL; @@ -2480,14 +2479,14 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, if ((rc = virCgroupKillInternal(group, signum, pids, controller, taskFile)) < 0) { - goto cleanup; + return -1; } if (rc == 1) killedAny = true; VIR_DEBUG("Iterate over children of %s (killedAny=%d)", keypath, killedAny); if ((rc = virDirOpenIfExists(&dp, keypath)) < 0) - goto cleanup; + return -1; if (rc == 0) { VIR_DEBUG("Path %s does not exist, assuming done", keypath); @@ -2504,11 +2503,11 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, VIR_DEBUG("Process subdir %s", ent->d_name); if (virCgroupNew(-1, ent->d_name, group, -1, &subgroup) < 0) - goto cleanup; + return -1; if ((rc = virCgroupKillRecursiveInternal(subgroup, signum, pids, controller, taskFile, true)) < 0) - goto cleanup; + return -1; if (rc == 1) killedAny = true; @@ -2516,13 +2515,10 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, virCgroupRemove(subgroup); } if (direrr < 0) - goto cleanup; + return -1; done: - ret = killedAny ? 1 : 0; - - cleanup: - return ret; + return killedAny ? 1 : 0; } diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 5ae2ad9ef9..9a4f3d7f32 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -456,7 +456,6 @@ virCommandMassCloseGetFDsLinux(virCommandPtr cmd G_GNUC_UNUSED, struct dirent *entry; const char *dirName = "/proc/self/fd"; int rc; - int ret = -1; if (virDirOpen(&dp, dirName) < 0) return -1; @@ -468,18 +467,16 @@ virCommandMassCloseGetFDsLinux(virCommandPtr cmd G_GNUC_UNUSED, virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to parse FD: %s"), entry->d_name); - goto cleanup; + return -1; } ignore_value(virBitmapSetBit(fds, fd)); } if (rc < 0) - goto cleanup; + return -1; - ret = 0; - cleanup: - return ret; + return 0; } # else /* !__linux__ */ diff --git a/src/util/virdevmapper.c b/src/util/virdevmapper.c index 323102d36e..6c39a2a44d 100644 --- a/src/util/virdevmapper.c +++ b/src/util/virdevmapper.c @@ -171,7 +171,6 @@ virDMSanitizepath(const char *path) struct stat sb[2]; g_autoptr(DIR) dh = NULL; const char *p; - char *ret = NULL; /* If a path is NOT provided then assume it's DM name */ p = strrchr(path, '/'); @@ -206,12 +205,11 @@ virDMSanitizepath(const char *path) if (stat(tmp, &sb[1]) == 0 && sb[0].st_rdev == sb[0].st_rdev) { - ret = g_steal_pointer(&tmp); - break; + return g_steal_pointer(&tmp); } } - return ret; + return NULL; } diff --git a/src/util/virfile.c b/src/util/virfile.c index fc0021036a..9a7dcfc9a6 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -836,7 +836,6 @@ static char * virFileNBDDeviceFindUnused(void) { g_autoptr(DIR) dh = NULL; - char *ret = NULL; struct dirent *de; int direrr; @@ -846,21 +845,19 @@ virFileNBDDeviceFindUnused(void) while ((direrr = virDirRead(dh, &de, SYSFS_BLOCK_DIR)) > 0) { if (STRPREFIX(de->d_name, "nbd")) { int rv = virFileNBDDeviceIsBusy(de->d_name); + if (rv < 0) - goto cleanup; - if (rv == 0) { - ret = g_strdup_printf("/dev/%s", de->d_name); - goto cleanup; - } + return NULL; + + if (rv == 0) + return g_strdup_printf("/dev/%s", de->d_name); } } if (direrr < 0) - goto cleanup; - virReportSystemError(EBUSY, "%s", - _("No free NBD devices")); + return NULL; - cleanup: - return ret; + virReportSystemError(EBUSY, "%s", _("No free NBD devices")); + return NULL; } static bool @@ -979,7 +976,6 @@ int virFileDeleteTree(const char *dir) { g_autoptr(DIR) dh = NULL; struct dirent *de; - int ret = -1; int direrr; /* Silently return 0 if passed NULL or directory doesn't exist */ @@ -998,35 +994,32 @@ int virFileDeleteTree(const char *dir) if (g_lstat(filepath, &sb) < 0) { virReportSystemError(errno, _("Cannot access '%s'"), filepath); - goto cleanup; + return -1; } if (S_ISDIR(sb.st_mode)) { if (virFileDeleteTree(filepath) < 0) - goto cleanup; + return -1; } else { if (unlink(filepath) < 0 && errno != ENOENT) { virReportSystemError(errno, _("Cannot delete file '%s'"), filepath); - goto cleanup; + return -1; } } } if (direrr < 0) - goto cleanup; + return -1; if (rmdir(dir) < 0 && errno != ENOENT) { virReportSystemError(errno, _("Cannot delete directory '%s'"), dir); - goto cleanup; + return -1; } - ret = 0; - - cleanup: - return ret; + return 0; } /* Like read(), but restarts after EINTR. Doesn't play @@ -2949,7 +2942,6 @@ int virFileChownFiles(const char *name, gid_t gid) { struct dirent *ent; - int ret = -1; int direrr; g_autoptr(DIR) dir = NULL; @@ -2969,17 +2961,14 @@ int virFileChownFiles(const char *name, _("cannot chown '%s' to (%u, %u)"), ent->d_name, (unsigned int) uid, (unsigned int) gid); - goto cleanup; + return -1; } } if (direrr < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } #else /* WIN32 */ diff --git a/src/util/virnetdev.c b/src/util/virnetdev.c index 256179c052..5104bbe7c5 100644 --- a/src/util/virnetdev.c +++ b/src/util/virnetdev.c @@ -2900,7 +2900,6 @@ virNetDevRDMAFeature(const char *ifname, g_autofree char *eth_res_buf = NULL; g_autoptr(DIR) dirp = NULL; struct dirent *dp; - int ret = -1; if (!virFileExists(SYSFS_INFINIBAND_DIR)) return 0; @@ -2913,12 +2912,11 @@ virNetDevRDMAFeature(const char *ifname, /* If /sys/class/net/<ifname>/device/resource doesn't exist it is not a PCI * device and therefore it will not have RDMA. */ if (!virFileExists(eth_devpath)) { - ret = 0; - goto cleanup; + return 0; } if (virFileReadAll(eth_devpath, RESOURCE_FILE_LEN, ð_res_buf) < 0) - goto cleanup; + return -1; while (virDirRead(dirp, &dp, SYSFS_INFINIBAND_DIR) > 0) { g_autofree char *ib_res_buf = NULL; @@ -2931,10 +2929,8 @@ virNetDevRDMAFeature(const char *ifname, break; } } - ret = 0; - cleanup: - return ret; + return 0; } diff --git a/src/util/virnuma.c b/src/util/virnuma.c index 51a5d6e8a6..a05e4ac72c 100644 --- a/src/util/virnuma.c +++ b/src/util/virnuma.c @@ -739,7 +739,6 @@ virNumaGetPages(int node, unsigned long long **pages_free, size_t *npages) { - int ret = -1; g_autoptr(DIR) dir = NULL; int direrr = 0; struct dirent *entry; @@ -763,12 +762,12 @@ virNumaGetPages(int node, * slightly different information. So we take the total memory on a node * and subtract memory taken by the huge pages. */ if (virNumaGetHugePageInfoDir(&path, node) < 0) - goto cleanup; + return -1; /* It's okay if the @path doesn't exist. Maybe we are running on * system without huge pages support where the path may not exist. */ if (virDirOpenIfExists(&dir, path) < 0) - goto cleanup; + return -1; while (dir && (direrr = virDirRead(dir, &entry, path)) > 0) { const char *page_name = entry->d_name; @@ -789,17 +788,17 @@ virNumaGetPages(int node, virReportError(VIR_ERR_INTERNAL_ERROR, _("unable to parse %s"), entry->d_name); - goto cleanup; + return -1; } if (virNumaGetHugePageInfo(node, page_size, &page_avail, &page_free) < 0) - goto cleanup; + return -1; if (VIR_REALLOC_N(tmp_size, ntmp + 1) < 0 || VIR_REALLOC_N(tmp_avail, ntmp + 1) < 0 || VIR_REALLOC_N(tmp_free, ntmp + 1) < 0) - goto cleanup; + return -1; tmp_size[ntmp] = page_size; tmp_avail[ntmp] = page_avail; @@ -812,17 +811,17 @@ virNumaGetPages(int node, } if (direrr < 0) - goto cleanup; + return -1; /* Now append the ordinary system pages */ if (VIR_REALLOC_N(tmp_size, ntmp + 1) < 0 || VIR_REALLOC_N(tmp_avail, ntmp + 1) < 0 || VIR_REALLOC_N(tmp_free, ntmp + 1) < 0) - goto cleanup; + return -1; if (virNumaGetPageInfo(node, system_page_size, huge_page_sum, &tmp_avail[ntmp], &tmp_free[ntmp]) < 0) - goto cleanup; + return -1; tmp_size[ntmp] = system_page_size; ntmp++; @@ -852,9 +851,7 @@ virNumaGetPages(int node, tmp_free = NULL; } *npages = ntmp; - ret = 0; - cleanup: - return ret; + return 0; } diff --git a/src/util/virpci.c b/src/util/virpci.c index 025eb9d91c..a501db6ff7 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1706,7 +1706,6 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, { g_autofree char *pcidir = NULL; g_autoptr(DIR) dir = NULL; - int ret = -1; struct dirent *ent; int direrr; @@ -1715,7 +1714,7 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, dev->address.function); if (virDirOpen(&dir, pcidir) < 0) - goto cleanup; + return -1; while ((direrr = virDirRead(dir, &ent, pcidir)) > 0) { g_autofree char *file = NULL; @@ -1731,16 +1730,13 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, STREQ(ent->d_name, "reset")) { file = g_strdup_printf("%s/%s", pcidir, ent->d_name); if ((actor)(dev, file, opaque) < 0) - goto cleanup; + return -1; } } if (direrr < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } @@ -1756,7 +1752,6 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, { g_autofree char *groupPath = NULL; g_autoptr(DIR) groupDir = NULL; - int ret = -1; struct dirent *ent; int direrr; @@ -1765,8 +1760,7 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, if (virDirOpenQuiet(&groupDir, groupPath) < 0) { /* just process the original device, nothing more */ - ret = (actor)(orig, opaque); - goto cleanup; + return (actor)(orig, opaque); } while ((direrr = virDirRead(groupDir, &ent, groupPath)) > 0) { @@ -1776,19 +1770,16 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, virReportError(VIR_ERR_INTERNAL_ERROR, _("Found invalid device link '%s' in '%s'"), ent->d_name, groupPath); - goto cleanup; + return -1; } if ((actor)(&newDev, opaque) < 0) - goto cleanup; + return -1; } if (direrr < 0) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index aa466592fc..d3087b98c1 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -786,23 +786,18 @@ virResctrlGetInfo(virResctrlInfoPtr resctrl) ret = virDirOpenIfExists(&dirp, SYSFS_RESCTRL_PATH "/info"); if (ret <= 0) - goto cleanup; + return ret; - ret = virResctrlGetMemoryBandwidthInfo(resctrl); - if (ret < 0) - goto cleanup; + if ((ret = virResctrlGetMemoryBandwidthInfo(resctrl)) < 0) + return -1; - ret = virResctrlGetCacheInfo(resctrl, dirp); - if (ret < 0) - goto cleanup; + if ((ret = virResctrlGetCacheInfo(resctrl, dirp)) < 0) + return -1; - ret = virResctrlGetMonitorInfo(resctrl); - if (ret < 0) - goto cleanup; + if ((ret = virResctrlGetMonitorInfo(resctrl)) < 0) + return -1; - ret = 0; - cleanup: - return ret; + return 0; } diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 256acc37fa..22d4679368 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -109,7 +109,6 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; - char *sg = NULL; unsigned int adapter_id; const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; @@ -120,16 +119,13 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, bus, target, unit); if (virDirOpen(&dir, path) < 0) - goto cleanup; + return NULL; - while (virDirRead(dir, &entry, path) > 0) { - /* Assume a single directory entry */ - sg = g_strdup(entry->d_name); - break; - } + /* Assume a single directory entry */ + if (virDirRead(dir, &entry, path) > 0) + return g_strdup(entry->d_name); - cleanup: - return sg; + return NULL; } /* Returns device name (e.g. "sdc") on success, or NULL @@ -145,7 +141,6 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; - char *name = NULL; unsigned int adapter_id; const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES; @@ -156,15 +151,12 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, target, unit); if (virDirOpen(&dir, path) < 0) - goto cleanup; + return NULL; - while (virDirRead(dir, &entry, path) > 0) { - name = g_strdup(entry->d_name); - break; - } + if (virDirRead(dir, &entry, path) > 0) + return g_strdup(entry->d_name); - cleanup: - return name; + return NULL; } virSCSIDevicePtr diff --git a/src/util/virutil.c b/src/util/virutil.c index 41e92023fc..a0cd0f1bcd 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1623,22 +1623,18 @@ virHostHasIOMMU(void) { g_autoptr(DIR) iommuDir = NULL; struct dirent *iommuGroup = NULL; - bool ret = false; int direrr; if (virDirOpenQuiet(&iommuDir, "/sys/kernel/iommu_groups/") < 0) - goto cleanup; + return false; while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) break; if (direrr < 0 || !iommuGroup) - goto cleanup; - - ret = true; + return false; - cleanup: - return ret; + return true; } @@ -1656,7 +1652,6 @@ virHostHasIOMMU(void) char * virHostGetDRMRenderNode(void) { - char *ret = NULL; g_autoptr(DIR) driDir = NULL; const char *driPath = "/dev/dri"; struct dirent *ent = NULL; @@ -1674,19 +1669,16 @@ virHostGetDRMRenderNode(void) } if (dirErr < 0) - goto cleanup; + return NULL; /* even if /dev/dri exists, there might be no renderDX nodes available */ if (!have_rendernode) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No DRM render nodes available")); - goto cleanup; + return NULL; } - ret = g_strdup_printf("%s/%s", driPath, ent->d_name); - - cleanup: - return ret; + return g_strdup_printf("%s/%s", driPath, ent->d_name); } diff --git a/src/util/virvhba.c b/src/util/virvhba.c index 471d94d3dd..a4e88024d1 100644 --- a/src/util/virvhba.c +++ b/src/util/virvhba.c @@ -365,7 +365,6 @@ virVHBAGetHostByWWN(const char *sysfs_prefix, const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; struct dirent *entry = NULL; g_autoptr(DIR) dir = NULL; - char *ret = NULL; if (virDirOpen(&dir, prefix) < 0) return NULL; @@ -375,24 +374,22 @@ virVHBAGetHostByWWN(const char *sysfs_prefix, if ((rc = vhbaReadCompareWWN(prefix, entry->d_name, "node_name", wwnn)) < 0) - goto cleanup; + return NULL; if (rc == 0) continue; if ((rc = vhbaReadCompareWWN(prefix, entry->d_name, "port_name", wwpn)) < 0) - goto cleanup; + return NULL; if (rc == 0) continue; - ret = g_strdup(entry->d_name); - break; + return g_strdup(entry->d_name); } - cleanup: - return ret; + return NULL; } /* virVHBAGetHostByFabricWWN: diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 5ae1d64337..c6848c12a2 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -516,12 +516,11 @@ testQemuGetLatestCapsForArch(const char *arch, unsigned long maxver = 0; unsigned long ver; g_autofree char *maxname = NULL; - char *ret = NULL; fullsuffix = g_strdup_printf("%s.%s", arch, suffix); if (virDirOpen(&dir, TEST_QEMU_CAPS_PATH) < 0) - goto cleanup; + return NULL; while ((rc = virDirRead(dir, &ent, TEST_QEMU_CAPS_PATH)) > 0) { g_autofree char *tmp = NULL; @@ -547,18 +546,15 @@ testQemuGetLatestCapsForArch(const char *arch, } if (rc < 0) - goto cleanup; + return NULL; if (!maxname) { VIR_TEST_VERBOSE("failed to find capabilities for '%s' in '%s'", arch, TEST_QEMU_CAPS_PATH); - goto cleanup; + return NULL; } - ret = g_strdup_printf("%s/%s", TEST_QEMU_CAPS_PATH, maxname); - - cleanup: - return ret; + return g_strdup_printf("%s/%s", TEST_QEMU_CAPS_PATH, maxname); } @@ -607,7 +603,6 @@ testQemuCapsIterate(const char *suffix, struct dirent *ent; g_autoptr(DIR) dir = NULL; int rc; - int ret = -1; bool fail = false; if (!callback) @@ -616,11 +611,11 @@ testQemuCapsIterate(const char *suffix, /* Validate suffix */ if (!STRPREFIX(suffix, ".")) { VIR_TEST_VERBOSE("malformed suffix '%s'", suffix); - goto cleanup; + return -1; } if (virDirOpen(&dir, TEST_QEMU_CAPS_PATH) < 0) - goto cleanup; + return -1; while ((rc = virDirRead(dir, &ent, TEST_QEMU_CAPS_PATH)) > 0) { g_autofree char *tmp = g_strdup(ent->d_name); @@ -634,13 +629,13 @@ testQemuCapsIterate(const char *suffix, /* Strip the leading prefix */ if (!(version = STRSKIP(tmp, "caps_"))) { VIR_TEST_VERBOSE("malformed file name '%s'", ent->d_name); - goto cleanup; + return -1; } /* Find the last dot */ if (!(archName = strrchr(tmp, '.'))) { VIR_TEST_VERBOSE("malformed file name '%s'", ent->d_name); - goto cleanup; + return -1; } /* The version number and the architecture name are separated by @@ -661,12 +656,9 @@ testQemuCapsIterate(const char *suffix, } if (rc < 0 || fail) - goto cleanup; - - ret = 0; + return -1; - cleanup: - return ret; + return 0; } -- 2.26.2
On 10/27/20 10:35 PM, Laine Stump wrote:
After converting all DIR* to g_autoptr(DIR), many cleanup: labels ended up just having "return ret", and every place that set ret would just immediately goto cleanup. Remove the cleanup label and its return, and just return the set value immediately, thus eliminating the need for the return variable itself.
Signed-off-by: Laine Stump <laine@redhat.com> --- src/conf/virnetworkobj.c | 32 +++++++--------------- src/qemu/qemu_interop_config.c | 11 +++----- src/storage/storage_util.c | 50 +++++++++++++--------------------- src/util/vircgroup.c | 16 ++++------- src/util/vircommand.c | 9 ++---- src/util/virdevmapper.c | 6 ++-- src/util/virfile.c | 45 ++++++++++++------------------ src/util/virnetdev.c | 10 ++----- src/util/virnuma.c | 21 ++++++-------- src/util/virpci.c | 27 ++++++------------ src/util/virresctrl.c | 21 ++++++-------- src/util/virscsi.c | 26 ++++++------------ src/util/virutil.c | 20 ++++---------- src/util/virvhba.c | 11 +++----- tests/testutilsqemu.c | 28 +++++++------------ 15 files changed, 119 insertions(+), 214 deletions(-)
[...]
diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 256acc37fa..22d4679368 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -109,7 +109,6 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; - char *sg = NULL; unsigned int adapter_id; const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES;
@@ -120,16 +119,13 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, bus, target, unit);
if (virDirOpen(&dir, path) < 0) - goto cleanup; + return NULL;
- while (virDirRead(dir, &entry, path) > 0) { - /* Assume a single directory entry */ - sg = g_strdup(entry->d_name); - break; - } + /* Assume a single directory entry */ + if (virDirRead(dir, &entry, path) > 0) + return g_strdup(entry->d_name);
Nit: extra whitespace after 'return' Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
- cleanup: - return sg; + return NULL; }
/* Returns device name (e.g. "sdc") on success, or NULL @@ -145,7 +141,6 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, g_autoptr(DIR) dir = NULL; struct dirent *entry; g_autofree char *path = NULL; - char *name = NULL; unsigned int adapter_id; const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_SCSI_DEVICES;
@@ -156,15 +151,12 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, target, unit);
if (virDirOpen(&dir, path) < 0) - goto cleanup; + return NULL;
- while (virDirRead(dir, &entry, path) > 0) { - name = g_strdup(entry->d_name); - break; - } + if (virDirRead(dir, &entry, path) > 0) + return g_strdup(entry->d_name);
- cleanup: - return name; + return NULL; }
virSCSIDevicePtr diff --git a/src/util/virutil.c b/src/util/virutil.c index 41e92023fc..a0cd0f1bcd 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1623,22 +1623,18 @@ virHostHasIOMMU(void) { g_autoptr(DIR) iommuDir = NULL; struct dirent *iommuGroup = NULL; - bool ret = false; int direrr;
if (virDirOpenQuiet(&iommuDir, "/sys/kernel/iommu_groups/") < 0) - goto cleanup; + return false;
while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) break;
if (direrr < 0 || !iommuGroup) - goto cleanup; - - ret = true; + return false;
- cleanup: - return ret; + return true; }
@@ -1656,7 +1652,6 @@ virHostHasIOMMU(void) char * virHostGetDRMRenderNode(void) { - char *ret = NULL; g_autoptr(DIR) driDir = NULL; const char *driPath = "/dev/dri"; struct dirent *ent = NULL; @@ -1674,19 +1669,16 @@ virHostGetDRMRenderNode(void) }
if (dirErr < 0) - goto cleanup; + return NULL;
/* even if /dev/dri exists, there might be no renderDX nodes available */ if (!have_rendernode) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("No DRM render nodes available")); - goto cleanup; + return NULL; }
- ret = g_strdup_printf("%s/%s", driPath, ent->d_name); - - cleanup: - return ret; + return g_strdup_printf("%s/%s", driPath, ent->d_name); }
diff --git a/src/util/virvhba.c b/src/util/virvhba.c index 471d94d3dd..a4e88024d1 100644 --- a/src/util/virvhba.c +++ b/src/util/virvhba.c @@ -365,7 +365,6 @@ virVHBAGetHostByWWN(const char *sysfs_prefix, const char *prefix = sysfs_prefix ? sysfs_prefix : SYSFS_FC_HOST_PATH; struct dirent *entry = NULL; g_autoptr(DIR) dir = NULL; - char *ret = NULL;
if (virDirOpen(&dir, prefix) < 0) return NULL; @@ -375,24 +374,22 @@ virVHBAGetHostByWWN(const char *sysfs_prefix,
if ((rc = vhbaReadCompareWWN(prefix, entry->d_name, "node_name", wwnn)) < 0) - goto cleanup; + return NULL;
if (rc == 0) continue;
if ((rc = vhbaReadCompareWWN(prefix, entry->d_name, "port_name", wwpn)) < 0) - goto cleanup; + return NULL;
if (rc == 0) continue;
- ret = g_strdup(entry->d_name); - break; + return g_strdup(entry->d_name); }
- cleanup: - return ret; + return NULL; }
/* virVHBAGetHostByFabricWWN: diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index 5ae1d64337..c6848c12a2 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -516,12 +516,11 @@ testQemuGetLatestCapsForArch(const char *arch, unsigned long maxver = 0; unsigned long ver; g_autofree char *maxname = NULL; - char *ret = NULL;
fullsuffix = g_strdup_printf("%s.%s", arch, suffix);
if (virDirOpen(&dir, TEST_QEMU_CAPS_PATH) < 0) - goto cleanup; + return NULL;
while ((rc = virDirRead(dir, &ent, TEST_QEMU_CAPS_PATH)) > 0) { g_autofree char *tmp = NULL; @@ -547,18 +546,15 @@ testQemuGetLatestCapsForArch(const char *arch, }
if (rc < 0) - goto cleanup; + return NULL;
if (!maxname) { VIR_TEST_VERBOSE("failed to find capabilities for '%s' in '%s'", arch, TEST_QEMU_CAPS_PATH); - goto cleanup; + return NULL; }
- ret = g_strdup_printf("%s/%s", TEST_QEMU_CAPS_PATH, maxname); - - cleanup: - return ret; + return g_strdup_printf("%s/%s", TEST_QEMU_CAPS_PATH, maxname); }
@@ -607,7 +603,6 @@ testQemuCapsIterate(const char *suffix, struct dirent *ent; g_autoptr(DIR) dir = NULL; int rc; - int ret = -1; bool fail = false;
if (!callback) @@ -616,11 +611,11 @@ testQemuCapsIterate(const char *suffix, /* Validate suffix */ if (!STRPREFIX(suffix, ".")) { VIR_TEST_VERBOSE("malformed suffix '%s'", suffix); - goto cleanup; + return -1; }
if (virDirOpen(&dir, TEST_QEMU_CAPS_PATH) < 0) - goto cleanup; + return -1;
while ((rc = virDirRead(dir, &ent, TEST_QEMU_CAPS_PATH)) > 0) { g_autofree char *tmp = g_strdup(ent->d_name); @@ -634,13 +629,13 @@ testQemuCapsIterate(const char *suffix, /* Strip the leading prefix */ if (!(version = STRSKIP(tmp, "caps_"))) { VIR_TEST_VERBOSE("malformed file name '%s'", ent->d_name); - goto cleanup; + return -1; }
/* Find the last dot */ if (!(archName = strrchr(tmp, '.'))) { VIR_TEST_VERBOSE("malformed file name '%s'", ent->d_name); - goto cleanup; + return -1; }
/* The version number and the architecture name are separated by @@ -661,12 +656,9 @@ testQemuCapsIterate(const char *suffix, }
if (rc < 0 || fail) - goto cleanup; - - ret = 0; + return -1;
- cleanup: - return ret; + return 0; }
Hey, On 10/27/20 10:35 PM, Laine Stump wrote:
I don't even remember why I started looking at this. I think that again I was considering changing some function, and making the DIR* in that function autoclose was a prerequisite for a reasonably clean addition to the function. I eventually gave up on the other plan (probably because it was a bad idea), but decided that the DIR*'s really did need to autoclose.
In the end, all uses of DIR* could be easily converted to use g_autoptr.
I think you created this series in parallel with your "node_device: fix leak of DIR*" patch, right?. Because you're not changing 'node_device_udev.c' anywhere in this series, meaning that the code base you used still have the DIR leak in udevGetVDPACharDev(). The code base didn't have the added VIR_DIR_CLOSE() calls there to fix the leak, and I believe you forgot to take that into account here. The end result is that the leak fix patch will break after patch 10 removes the VIR_DIR_CLOSE() macro. A quick fix is simply using "node_device: fix leak of DIR*" as the first patch of this series, then you can handle the removal of VIR_DIR_CLOSE() and g_autoptr() for that case in patch 8. There's no cleanup labels to be added there, so it's a trivial removal of the two VIR_DIR_CLOSE() calls and turning the DIR var to g_autoptr(). Assuming you go with that (and fix my whitespace nit in patch 12! :P ), feel free to add my R-b in all patches: Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com> Thanks, DHB
Laine Stump (12): consistently use VIR_DIR_CLOSE() instead of virDirClose() tools: reduce scope of a DIR* in virHostValidateIOMMU() storage: remove extraneous call to VIR_DIR_CLOSE() util: reduce scope of a DIR * in virCgroupV1SetOwner() util: manually set dirp to NULL after closing in virCapabilitiesInitCache() util: change virDirClose to take a DIR* instead of DIR**. util: declare g_autoptr cleanup function to auto-close DIR* change DIR* int g_autoptr(DIR) where appropriate conf: convert final DIR* to g_autoptr util: remove unused VIR_DIR_CLOSE() macro util: refactor function to simplify and remove label remove unnecessary cleanup labels and unused return variables
src/bhyve/bhyve_capabilities.c | 3 +- src/conf/capabilities.c | 5 +- src/conf/virdomainobjlist.c | 3 +- src/conf/virnetworkobj.c | 44 +++++--------- src/conf/virnwfilterbindingobjlist.c | 3 +- src/conf/virnwfilterobj.c | 3 +- src/conf/virsecretobj.c | 3 +- src/conf/virstorageobj.c | 6 +- src/openvz/openvz_conf.c | 3 +- src/qemu/qemu_driver.c | 6 +- src/qemu/qemu_interop_config.c | 14 ++--- src/security/security_selinux.c | 8 +-- src/storage/storage_backend_iscsi.c | 3 +- src/storage/storage_util.c | 69 ++++++++------------- src/util/vircgroup.c | 23 +++---- src/util/vircgroupv1.c | 5 +- src/util/vircommand.c | 12 ++-- src/util/virdevmapper.c | 9 +-- src/util/virfile.c | 65 ++++++++------------ src/util/virfile.h | 4 +- src/util/virhook.c | 8 +-- src/util/virhostcpu.c | 6 +- src/util/virnetdev.c | 13 ++-- src/util/virnuma.c | 24 +++----- src/util/virpci.c | 91 +++++++++++----------------- src/util/virprocess.c | 3 +- src/util/virresctrl.c | 30 ++++----- src/util/virscsi.c | 32 ++++------ src/util/virscsihost.c | 3 +- src/util/virusb.c | 3 +- src/util/virutil.c | 26 +++----- src/util/virvhba.c | 20 +++--- tests/testutilsqemu.c | 35 ++++------- tests/virschematest.c | 3 +- tools/virt-host-validate-common.c | 4 +- 35 files changed, 206 insertions(+), 386 deletions(-)
On 10/28/20 7:39 AM, Daniel Henrique Barboza wrote:
Hey,
On 10/27/20 10:35 PM, Laine Stump wrote:
I don't even remember why I started looking at this. I think that again I was considering changing some function, and making the DIR* in that function autoclose was a prerequisite for a reasonably clean addition to the function. I eventually gave up on the other plan (probably because it was a bad idea), but decided that the DIR*'s really did need to autoclose.
In the end, all uses of DIR* could be easily converted to use g_autoptr.
I think you created this series in parallel with your "node_device: fix leak of DIR*" patch, right?.
No. This series has been building up for a couple weeks, but the leak just came up 2 days ago and I didn't write the patch for it until I had finished and posted this series. But the node-device bugfix needs to be pushed before the next release, while this series won't be pushed until after the release, so the bugfix had to be based on the current state of upstream, rather than stacked on top of other work.
Because you're not changing 'node_device_udev.c' anywhere in this series, meaning that the code base you used still have the DIR leak in udevGetVDPACharDev(). The code base didn't have the added VIR_DIR_CLOSE() calls there to fix the leak, and I believe you forgot to take that into account here. The end result is that the leak fix patch will break after patch 10 removes the VIR_DIR_CLOSE() macro.
Yep. It wasn't forgotten, just done in a different order than you assumed, and both had to be based on current upstream rather than dependent on each other.
A quick fix is simply using "node_device: fix leak of DIR*" as the first patch of this series, then you can handle the removal of VIR_DIR_CLOSE() and g_autoptr() for that case in patch 8.
Yep, that's what I'd planned to do.
There's no cleanup labels to be added there, so it's a trivial removal of the two VIR_DIR_CLOSE() calls and turning the DIR var to g_autoptr().
Assuming you go with that (and fix my whitespace nit in patch 12! :P ), feel free to add my R-b in all patches:
Okay, thanks!
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
participants (2)
-
Daniel Henrique Barboza -
Laine Stump