[libvirt] [PATCHv2 0/7] saner readdir usage
This extends the work started by Nataneal to cover the entire tree. Basically, readdir() is awkward to use correctly: to properly check for (unlikely) errors, one must pre-set errno. Forcing everything to go through a wrapper ensures that all our uses will be consistent; this also fixes several bugs in some of the call sites. Eric Blake (5): conf: use virDirRead API util: use virDirRead API drivers: use virDirRead API storage: use virDirRead API enforce sane readdir usage Natanael Copa (2): util: introduce virDirRead wrapper for readdir nodeinfo: use virDirRead API cfg.mk | 6 +++ src/conf/domain_conf.c | 5 ++- src/conf/network_conf.c | 11 +++--- src/conf/nwfilter_conf.c | 6 +-- src/conf/storage_conf.c | 6 +-- src/libvirt_private.syms | 1 + src/nodeinfo.c | 31 +++++---------- src/openvz/openvz_conf.c | 9 +---- src/parallels/parallels_storage.c | 11 ++++-- src/qemu/qemu_driver.c | 5 ++- src/qemu/qemu_hostdev.c | 7 ++-- src/secret/secret_driver.c | 6 +-- src/storage/storage_backend.c | 8 ++-- src/storage/storage_backend_fs.c | 6 ++- src/storage/storage_backend_iscsi.c | 5 ++- src/storage/storage_backend_scsi.c | 10 +++-- src/util/vircgroup.c | 28 ++++++++------ src/util/virfile.c | 76 +++++++++++++++++++++++-------------- src/util/virfile.h | 4 ++ src/util/virnetdevtap.c | 10 +---- src/util/virpci.c | 20 +++++----- src/util/virscsi.c | 9 +++-- src/util/virusb.c | 7 +++- src/util/virutil.c | 4 +- src/xen/xen_inotify.c | 7 +++- src/xen/xm_internal.c | 3 +- 26 files changed, 170 insertions(+), 131 deletions(-) -- 1.9.0
From: Natanael Copa <ncopa@alpinelinux.org> Introduce a wrapper for readdir. This helps us make sure that we always set errno before calling readdir and it will make sure errors are properly logged. Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 33 +++++++++++++++++++++++++++++++++ src/util/virfile.h | 4 ++++ 3 files changed, 38 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 5788468..8bbe6e7 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1222,6 +1222,7 @@ safewrite; safezero; virBuildPathInternal; virDirCreate; +virDirRead; virFileAbsPath; virFileAccessibleAs; virFileActivateDirOverride; diff --git a/src/util/virfile.c b/src/util/virfile.c index da4e0a0..3481124 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2391,6 +2391,39 @@ virDirCreate(const char *path ATTRIBUTE_UNUSED, } #endif /* WIN32 */ +/** + * virDirRead: + * @dirp: directory to read + * @end: output one entry + * @name: if non-NULL, the name related to @dirp for use in error reporting + * + * Wrapper around readdir. Typical usage: + * struct dirent ent; + * int value; + * DIR *dir; + * if (!(dir = opendir(name))) + * goto error; + * while ((value = virDirRead(dir, &ent, name)) > 0) + * process ent; + * if (value < 0) + * goto error; + * + * Returns -1 on error, with error already reported if @name was + * supplied. On success, returns 1 for entry read, 0 for end-of-dir. + */ +int virDirRead(DIR *dirp, struct dirent **ent, const char *name) +{ + errno = 0; + *ent = readdir(dirp); + if (!*ent && errno) { + if (name) + virReportSystemError(errno, _("Unable to read directory '%s'"), + name); + return -1; + } + return !!*ent; +} + static int virFileMakePathHelper(char *path, mode_t mode) { diff --git a/src/util/virfile.h b/src/util/virfile.h index 5d0d699..12bcb22 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -27,6 +27,7 @@ # define __VIR_FILE_H_ # include <stdio.h> +# include <dirent.h> # include "internal.h" # include "virstoragefile.h" @@ -225,6 +226,9 @@ enum { }; int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) ATTRIBUTE_RETURN_CHECK; +int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_RETURN_CHECK; + int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK; int virFileMakePathWithMode(const char *path, mode_t mode) ATTRIBUTE_RETURN_CHECK; -- 1.9.0
On 04/26/2014 03:18 PM, Eric Blake wrote:
From: Natanael Copa <ncopa@alpinelinux.org>
Introduce a wrapper for readdir. This helps us make sure that we always set errno before calling readdir and it will make sure errors are properly logged.
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virfile.c | 33 +++++++++++++++++++++++++++++++++ src/util/virfile.h | 4 ++++ 3 files changed, 38 insertions(+)
ACK Jan
From: Natanael Copa <ncopa@alpinelinux.org> This makes sure that errno is reset before readdir is called, even if the loop does a 'continue'. This fixes issue with musl libc which sets errno on sscanf. The following 'continue' makes the errno be set before calling readdir. Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/nodeinfo.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 53ba716..56f2b02 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1,7 +1,7 @@ /* * nodeinfo.c: Helper routines for OS specific node information * - * Copyright (C) 2006-2008, 2010-2013 Red Hat, Inc. + * Copyright (C) 2006-2008, 2010-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -440,6 +440,7 @@ virNodeParseNode(const char *node, int siblings; unsigned int cpu; int online; + int direrr; *threads = 0; *cores = 0; @@ -452,8 +453,7 @@ virNodeParseNode(const char *node, /* enumerate sockets in the node */ CPU_ZERO(&sock_map); - errno = 0; - while ((cpudirent = readdir(cpudir))) { + while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; @@ -470,14 +470,10 @@ virNodeParseNode(const char *node, if (sock > sock_max) sock_max = sock; - - errno = 0; } - if (errno) { - virReportSystemError(errno, _("problem reading %s"), node); + if (direrr < 0) goto cleanup; - } sock_max++; @@ -490,8 +486,7 @@ virNodeParseNode(const char *node, /* iterate over all CPU's in the node */ rewinddir(cpudir); - errno = 0; - while ((cpudirent = readdir(cpudir))) { + while ((direrr = virDirRead(cpudir, &cpudirent, node)) > 0) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; @@ -530,14 +525,10 @@ virNodeParseNode(const char *node, if (siblings > *threads) *threads = siblings; - - errno = 0; } - if (errno) { - virReportSystemError(errno, _("problem reading %s"), node); + if (direrr < 0) goto cleanup; - } /* finalize the returned data */ *sockets = CPU_COUNT(&sock_map); @@ -576,6 +567,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, int ret = -1; char *sysfs_nodedir = NULL; char *sysfs_cpudir = NULL; + int direrr; /* Start with parsing CPU clock speed from /proc/cpuinfo */ while (fgets(line, sizeof(line), cpuinfo) != NULL) { @@ -672,8 +664,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, goto fallback; } - errno = 0; - while ((nodedirent = readdir(nodedir))) { + while ((direrr = virDirRead(nodedir, &nodedirent, sysfs_nodedir)) > 0) { if (sscanf(nodedirent->d_name, "node%u", &node) != 1) continue; @@ -699,14 +690,10 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, if (threads > nodeinfo->threads) nodeinfo->threads = threads; - - errno = 0; } - if (errno) { - virReportSystemError(errno, _("problem reading %s"), sysfs_nodedir); + if (direrr < 0) goto cleanup; - } if (nodeinfo->cpus && nodeinfo->nodes) goto done; -- 1.9.0
On 04/26/2014 03:18 PM, Eric Blake wrote:
From: Natanael Copa <ncopa@alpinelinux.org>
This makes sure that errno is reset before readdir is called, even if the loop does a 'continue'.
This fixes issue with musl libc which sets errno on sscanf. The following 'continue' makes the errno be set before calling readdir.
Signed-off-by: Natanael Copa <ncopa@alpinelinux.org> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/nodeinfo.c | 31 +++++++++---------------------- 1 file changed, 9 insertions(+), 22 deletions(-)
ACK Jan
When reading configuration files, we were silently ignoring directory read failures. While unlikely, we might as well report them. * src/conf/domain_conf.c (virDomainObjListLoadAllConfigs): Report readdir errors. * src/conf/network_conf.c (virNetworkLoadAllState) (virNetworkLoadAllConfigs): Likewise. * src/conf/nwfilter_conf.c (virNWFilterLoadAllConfigs): Likewise. * src/conf/storage_conf.c (virStoragePoolLoadAllConfigs): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 5 +++-- src/conf/network_conf.c | 11 ++++++----- src/conf/nwfilter_conf.c | 6 +++--- src/conf/storage_conf.c | 6 +++--- 4 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index c655bcf..0569f66 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -18219,6 +18219,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, { DIR *dir; struct dirent *entry; + int ret = -1; VIR_INFO("Scanning for configs in %s", configDir); @@ -18233,7 +18234,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, virObjectLock(doms); - while ((entry = readdir(dir))) { + while ((ret = virDirRead(dir, &entry, configDir)) > 0) { virDomainObjPtr dom; if (entry->d_name[0] == '.') @@ -18273,7 +18274,7 @@ virDomainObjListLoadAllConfigs(virDomainObjListPtr doms, closedir(dir); virObjectUnlock(doms); - return 0; + return ret; } int diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 56c4a09..94ebaf8 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3139,6 +3139,7 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, { DIR *dir; struct dirent *entry; + int ret = -1; if (!(dir = opendir(stateDir))) { if (errno == ENOENT) @@ -3148,7 +3149,7 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, return -1; } - while ((entry = readdir(dir))) { + while ((ret = virDirRead(dir, &entry, stateDir)) > 0) { virNetworkObjPtr net; if (entry->d_name[0] == '.') @@ -3162,7 +3163,7 @@ virNetworkLoadAllState(virNetworkObjListPtr nets, } closedir(dir); - return 0; + return ret; } @@ -3172,6 +3173,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, { DIR *dir; struct dirent *entry; + int ret = -1; if (!(dir = opendir(configDir))) { if (errno == ENOENT) @@ -3182,7 +3184,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, return -1; } - while ((entry = readdir(dir))) { + while ((ret = virDirRead(dir, &entry, configDir)) > 0) { virNetworkObjPtr net; if (entry->d_name[0] == '.') @@ -3202,8 +3204,7 @@ int virNetworkLoadAllConfigs(virNetworkObjListPtr nets, } closedir(dir); - - return 0; + return ret; } int virNetworkDeleteConfig(const char *configDir, diff --git a/src/conf/nwfilter_conf.c b/src/conf/nwfilter_conf.c index b332f0f..90da0b5 100644 --- a/src/conf/nwfilter_conf.c +++ b/src/conf/nwfilter_conf.c @@ -3097,6 +3097,7 @@ virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters, { DIR *dir; struct dirent *entry; + int ret = -1; if (!(dir = opendir(configDir))) { if (errno == ENOENT) { @@ -3107,7 +3108,7 @@ virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters, return -1; } - while ((entry = readdir(dir))) { + while ((ret = virDirRead(dir, &entry, configDir)) > 0) { char *path; virNWFilterObjPtr nwfilter; @@ -3128,8 +3129,7 @@ virNWFilterLoadAllConfigs(virNWFilterObjListPtr nwfilters, } closedir(dir); - - return 0; + return ret; } diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ebd42c2..9769b19 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1862,6 +1862,7 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, { DIR *dir; struct dirent *entry; + int ret; if (!(dir = opendir(configDir))) { if (errno == ENOENT) @@ -1871,7 +1872,7 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, return -1; } - while ((entry = readdir(dir))) { + while ((ret = virDirRead(dir, &entry, configDir)) > 0) { char *path; char *autostartLink; virStoragePoolObjPtr pool; @@ -1901,8 +1902,7 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, } closedir(dir); - - return 0; + return ret; } int -- 1.9.0
In making the conversion to the new API, I fixed a couple bugs: virSCSIDeviceGetSgName would leak memory if a directory unexpectedly contained multiple entries; virNetDevTapGetRealDeviceName could report a spurious error from a stale errno inherited before starting the readdir search. The decision on whether to store the result of virDirRead into a variable is based on whether the end of the loop falls through to cleanup code automatically. In some cases, we have loops that are documented to return NULL on failure, and which raise an error on most failure paths but not in the case where the directory was unexpectedly empty; it may be worth a followup patch to explicitly report an error if readdir was successful but the directory was empty, so that a NULL return always has an error set. * src/util/vircgroup.c (virCgroupRemoveRecursively): Use new interface. (virCgroupKillRecursiveInternal, virCgroupSetOwner): Report readdir failures. * src/util/virfile.c (virFileLoopDeviceOpenSearch) (virFileNBDDeviceFindUnused, virFileDeleteTree): Use new interface. * src/util/virnetdevtap.c (virNetDevTapGetRealDeviceName): Properly check readdir errors. * src/util/virpci.c (virPCIDeviceIterDevices) (virPCIDeviceFileIterate, virPCIGetNetName): Report readdir failures. (virPCIDeviceAddressIOMMUGroupIterate): Use new interface. * src/util/virscsi.c (virSCSIDeviceGetSgName): Report readdir failures, and avoid memory leak. (virSCSIDeviceGetDevName): Report readdir failures. * src/util/virusb.c (virUSBDeviceSearch): Report readdir failures. * src/util/virutil.c (virGetFCHostNameByWWN) (virFindFCHostCapableVport): Report readdir failures. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/util/vircgroup.c | 28 +++++++++++++++++----------- src/util/virfile.c | 43 +++++++++++++++---------------------------- src/util/virnetdevtap.c | 10 ++-------- src/util/virpci.c | 20 ++++++++++---------- src/util/virscsi.c | 9 +++++---- src/util/virusb.c | 7 +++++-- src/util/virutil.c | 4 ++-- 7 files changed, 56 insertions(+), 65 deletions(-) diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index c7ebf8d..fce380a 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3137,6 +3137,7 @@ virCgroupRemoveRecursively(char *grppath) DIR *grpdir; struct dirent *ent; int rc = 0; + int direrr; grpdir = opendir(grppath); if (grpdir == NULL) { @@ -3147,17 +3148,11 @@ virCgroupRemoveRecursively(char *grppath) return rc; } - for (;;) { + /* 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; - errno = 0; - ent = readdir(grpdir); - if (ent == NULL) { - if ((rc = -errno)) - VIR_ERROR(_("Failed to readdir for %s (%d)"), grppath, errno); - break; - } - if (ent->d_name[0] == '.') continue; if (ent->d_type != DT_DIR) continue; @@ -3170,6 +3165,11 @@ virCgroupRemoveRecursively(char *grppath) if (rc != 0) break; } + if (direrr < 0) { + rc = -errno; + VIR_ERROR(_("Failed to readdir for %s (%d)"), grppath, errno); + } + closedir(grpdir); VIR_DEBUG("Removing cgroup %s", grppath); @@ -3373,6 +3373,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, DIR *dp; virCgroupPtr subgroup = NULL; struct dirent *ent; + int direrr; VIR_DEBUG("group=%p path=%s signum=%d pids=%p", group, group->path, signum, pids); @@ -3396,7 +3397,7 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, return -1; } - while ((ent = readdir(dp))) { + while ((direrr = virDirRead(dp, &ent, keypath)) > 0) { if (STREQ(ent->d_name, ".")) continue; if (STREQ(ent->d_name, "..")) @@ -3420,6 +3421,8 @@ virCgroupKillRecursiveInternal(virCgroupPtr group, virCgroupFree(&subgroup); } + if (direrr < 0) + goto cleanup; done: ret = killedAny ? 1 : 0; @@ -3701,6 +3704,7 @@ int virCgroupSetOwner(virCgroupPtr cgroup, size_t i; char *base = NULL, *entry = NULL; DIR *dh = NULL; + int direrr; for (i = 0; i < VIR_CGROUP_CONTROLLER_LAST; i++) { struct dirent *de; @@ -3721,7 +3725,7 @@ int virCgroupSetOwner(virCgroupPtr cgroup, goto cleanup; } - while ((de = readdir(dh)) != NULL) { + while ((direrr = virDirRead(dh, &de, base)) > 0) { if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) continue; @@ -3738,6 +3742,8 @@ int virCgroupSetOwner(virCgroupPtr cgroup, VIR_FREE(entry); } + if (direrr < 0) + goto cleanup; if (chown(base, uid, gid) < 0) { virReportSystemError(errno, diff --git a/src/util/virfile.c b/src/util/virfile.c index 3481124..bb1e7ba 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -607,6 +607,7 @@ static int virFileLoopDeviceOpenSearch(char **dev_name) struct dirent *de; char *looppath = NULL; struct loop_info64 lo; + int direrr; VIR_DEBUG("Looking for loop devices in /dev"); @@ -616,8 +617,7 @@ static int virFileLoopDeviceOpenSearch(char **dev_name) goto cleanup; } - errno = 0; - while ((de = readdir(dh)) != NULL) { + while ((direrr = virDirRead(dh, &de, "/dev")) > 0) { /* Checking 'loop' prefix is insufficient, since * new kernels have a dev named 'loop-control' */ @@ -650,15 +650,11 @@ static int virFileLoopDeviceOpenSearch(char **dev_name) /* Oh well, try the next device */ VIR_FORCE_CLOSE(fd); VIR_FREE(looppath); - errno = 0; } - - if (errno != 0) - virReportSystemError(errno, "%s", - _("Unable to iterate over loop devices")); - else - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Unable to find a free loop device in /dev")); + if (direrr < 0) + goto cleanup; + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unable to find a free loop device in /dev")); cleanup: if (fd != -1) { @@ -781,6 +777,7 @@ virFileNBDDeviceFindUnused(void) DIR *dh; char *ret = NULL; struct dirent *de; + int direrr; if (!(dh = opendir(SYSFS_BLOCK_DIR))) { virReportSystemError(errno, @@ -789,8 +786,7 @@ virFileNBDDeviceFindUnused(void) return NULL; } - errno = 0; - while ((de = readdir(dh)) != NULL) { + while ((direrr = virDirRead(dh, &de, SYSFS_BLOCK_DIR)) > 0) { if (STRPREFIX(de->d_name, "nbd")) { int rv = virFileNBDDeviceIsBusy(de->d_name); if (rv < 0) @@ -801,15 +797,11 @@ virFileNBDDeviceFindUnused(void) goto cleanup; } } - errno = 0; } - - if (errno != 0) - virReportSystemError(errno, "%s", - _("Unable to iterate over NBD devices")); - else - virReportSystemError(EBUSY, "%s", - _("No free NBD devices")); + if (direrr < 0) + goto cleanup; + virReportSystemError(EBUSY, "%s", + _("No free NBD devices")); cleanup: closedir(dh); @@ -918,6 +910,7 @@ int virFileDeleteTree(const char *dir) struct dirent *de; char *filepath = NULL; int ret = -1; + int direrr; if (!dh) { virReportSystemError(errno, _("Cannot open dir '%s'"), @@ -925,8 +918,7 @@ int virFileDeleteTree(const char *dir) return -1; } - errno = 0; - while ((de = readdir(dh)) != NULL) { + while ((direrr = virDirRead(dh, &de, dir)) > 0) { struct stat sb; if (STREQ(de->d_name, ".") || @@ -956,14 +948,9 @@ int virFileDeleteTree(const char *dir) } VIR_FREE(filepath); - errno = 0; } - - if (errno) { - virReportSystemError(errno, _("Cannot read dir '%s'"), - dir); + if (direrr < 0) goto cleanup; - } if (rmdir(dir) < 0 && errno != ENOENT) { virReportSystemError(errno, diff --git a/src/util/virnetdevtap.c b/src/util/virnetdevtap.c index 03bb418..0b444fa 100644 --- a/src/util/virnetdevtap.c +++ b/src/util/virnetdevtap.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2007-2013 Red Hat, Inc. + * Copyright (C) 2007-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -105,7 +105,7 @@ virNetDevTapGetRealDeviceName(char *ifname ATTRIBUTE_UNUSED) return NULL; } - while ((dp = readdir(dirp)) != NULL) { + while (virDirRead(dirp, &dp, "/dev") > 0) { if (STRPREFIX(dp->d_name, "tap")) { struct ifreq ifr; if (virAsprintf(&devpath, "/dev/%s", dp->d_name) < 0) { @@ -139,14 +139,8 @@ virNetDevTapGetRealDeviceName(char *ifname ATTRIBUTE_UNUSED) VIR_FREE(devpath); VIR_FORCE_CLOSE(fd); } - - errno = 0; } - if (errno != 0) - virReportSystemError(errno, "%s", - _("Unable to iterate over TAP devices")); - cleanup: VIR_FREE(devpath); VIR_FORCE_CLOSE(fd); diff --git a/src/util/virpci.c b/src/util/virpci.c index af0bfa8..e0f2344 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -1,7 +1,7 @@ /* * virpci.c: helper APIs for managing host PCI devices * - * Copyright (C) 2009-2013 Red Hat, Inc. + * Copyright (C) 2009-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -445,7 +445,7 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate, return -1; } - while ((entry = readdir(dir))) { + while ((ret = virDirRead(dir, &entry, PCI_SYSFS "devices")) > 0) { unsigned int domain, bus, slot, function; virPCIDevicePtr check; char *tmp; @@ -1907,6 +1907,7 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, DIR *dir = NULL; int ret = -1; struct dirent *ent; + int direrr; if (virAsprintf(&pcidir, "/sys/bus/pci/devices/%04x:%02x:%02x.%x", dev->domain, dev->bus, dev->slot, dev->function) < 0) @@ -1918,7 +1919,7 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, goto cleanup; } - while ((ent = readdir(dir)) != NULL) { + while ((direrr = virDirRead(dir, &ent, pcidir)) > 0) { /* Device assignment requires: * $PCIDIR/config, $PCIDIR/resource, $PCIDIR/resourceNNN, * $PCIDIR/rom, $PCIDIR/reset @@ -1935,6 +1936,8 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev, VIR_FREE(file); } } + if (direrr < 0) + goto cleanup; ret = 0; @@ -1961,6 +1964,7 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, DIR *groupDir = NULL; int ret = -1; struct dirent *ent; + int direrr; if (virAsprintf(&groupPath, PCI_SYSFS "devices/%04x:%02x:%02x.%x/iommu_group/devices", @@ -1973,7 +1977,7 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, goto cleanup; } - while ((errno = 0, ent = readdir(groupDir)) != NULL) { + while ((direrr = virDirRead(groupDir, &ent, groupPath)) > 0) { virPCIDeviceAddress newDev; if (ent->d_name[0] == '.') @@ -1989,12 +1993,8 @@ virPCIDeviceAddressIOMMUGroupIterate(virPCIDeviceAddressPtr orig, if ((actor)(&newDev, opaque) < 0) goto cleanup; } - if (errno != 0) { - virReportSystemError(errno, - _("Failed to read directory entry for %s"), - groupPath); + if (direrr < 0) goto cleanup; - } ret = 0; @@ -2638,7 +2638,7 @@ virPCIGetNetName(char *device_link_sysfs_path, char **netname) if (dir == NULL) goto out; - while ((entry = readdir(dir))) { + while (virDirRead(dir, &entry, pcidev_sysfs_net_path) > 0) { if (STREQ(entry->d_name, ".") || STREQ(entry->d_name, "..")) continue; diff --git a/src/util/virscsi.c b/src/util/virscsi.c index 572b818..9a0205f 100644 --- a/src/util/virscsi.c +++ b/src/util/virscsi.c @@ -133,12 +133,13 @@ virSCSIDeviceGetSgName(const char *sysfs_prefix, goto cleanup; } - while ((entry = readdir(dir))) { + while (virDirRead(dir, &entry, path) > 0) { if (entry->d_name[0] == '.') continue; - if (VIR_STRDUP(sg, entry->d_name) < 0) - goto cleanup; + /* Assume a single directory entry */ + ignore_value(VIR_STRDUP(sg, entry->d_name)); + break; } cleanup: @@ -178,7 +179,7 @@ virSCSIDeviceGetDevName(const char *sysfs_prefix, goto cleanup; } - while ((entry = readdir(dir))) { + while (virDirRead(dir, &entry, path) > 0) { if (entry->d_name[0] == '.') continue; diff --git a/src/util/virusb.c b/src/util/virusb.c index d959f5f..8244771 100644 --- a/src/util/virusb.c +++ b/src/util/virusb.c @@ -1,7 +1,7 @@ /* * virusb.c: helper APIs for managing host USB devices * - * Copyright (C) 2009-2013 Red Hat, Inc. + * Copyright (C) 2009-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -134,6 +134,7 @@ virUSBDeviceSearch(unsigned int vendor, struct dirent *de; virUSBDeviceListPtr list = NULL, ret = NULL; virUSBDevicePtr usb; + int direrr; if (!(list = virUSBDeviceListNew())) goto cleanup; @@ -146,7 +147,7 @@ virUSBDeviceSearch(unsigned int vendor, goto cleanup; } - while ((de = readdir(dir))) { + while ((direrr = virDirRead(dir, &de, USB_SYSFS "/devices")) > 0) { unsigned int found_prod, found_vend, found_bus, found_devno; char *tmpstr = de->d_name; @@ -197,6 +198,8 @@ virUSBDeviceSearch(unsigned int vendor, if (found) break; } + if (direrr < 0) + goto cleanup; ret = list; cleanup: diff --git a/src/util/virutil.c b/src/util/virutil.c index 9be1590..875e2b0 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -1867,7 +1867,7 @@ virGetFCHostNameByWWN(const char *sysfs_prefix, p = buf; \ } while (0) - while ((entry = readdir(dir))) { + while (virDirRead(dir, &entry, prefix) > 0) { if (entry->d_name[0] == '.') continue; @@ -1949,7 +1949,7 @@ virFindFCHostCapableVport(const char *sysfs_prefix) return NULL; } - while ((entry = readdir(dir))) { + while (virDirRead(dir, &entry, prefix) > 0) { unsigned int host; char *p = NULL; -- 1.9.0
Convert all remaining clients of readdir to use the new interface, so that we can ensure (unlikely) errors while reading a directory are reported. * src/openvz/openvz_conf.c (openvzAssignUUIDs): Use new interface. * src/parallels/parallels_storage.c (parallelsFindVolumes) (parallelsFindVmVolumes): Report readdir failures. * src/qemu/qemu_driver.c (qemuDomainSnapshotLoad): Ignore readdir failures. * src/secret/secret_driver.c (loadSecrets): Likewise. * src/qemu/qemu_hostdev.c (qemuHostdevHostSupportsPassthroughVFIO): Report readdir failures. * src/xen/xen_inotify.c (xenInotifyOpen): Likewise. * src/xen/xm_internal.c (xenXMConfigCacheRefresh): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/openvz/openvz_conf.c | 9 +-------- src/parallels/parallels_storage.c | 11 ++++++++--- src/qemu/qemu_driver.c | 5 ++++- src/qemu/qemu_hostdev.c | 7 ++++--- src/secret/secret_driver.c | 6 +++--- src/xen/xen_inotify.c | 7 +++++-- src/xen/xm_internal.c | 3 +-- 7 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index a7aff2a..dc84b29 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -1105,20 +1105,13 @@ static int openvzAssignUUIDs(void) return 0; } - errno = 0; - while ((dent = readdir(dp))) { + while ((ret = virDirRead(dp, &dent, conf_dir)) > 0) { if (virStrToLong_i(dent->d_name, &ext, 10, &vpsid) < 0 || *ext++ != '.' || STRNEQ(ext, "conf")) continue; if (vpsid > 0) /* '0.conf' belongs to the host, ignore it */ openvzSetUUID(vpsid); - errno = 0; - } - if (errno) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Failed to scan configuration directory")); - ret = -1; } closedir(dp); diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index 736d060..4dbaed1 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -93,6 +93,7 @@ parallelsFindVolumes(virStoragePoolObjPtr pool) struct dirent *ent; char *path = NULL; int ret = -1; + int direrr; if (!(dir = opendir(pool->def->target.path))) { virReportSystemError(errno, @@ -101,7 +102,7 @@ parallelsFindVolumes(virStoragePoolObjPtr pool) return -1; } - while ((ent = readdir(dir)) != NULL) { + while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) { if (!virFileHasSuffix(ent->d_name, ".xml")) continue; @@ -113,6 +114,8 @@ parallelsFindVolumes(virStoragePoolObjPtr pool) VIR_FREE(path); } + if (direrr < 0) + goto cleanup; ret = 0; cleanup: @@ -331,6 +334,7 @@ static int parallelsFindVmVolumes(virStoragePoolObjPtr pool, char *diskPath = NULL, *diskDescPath = NULL; struct stat sb; int ret = -1; + int direrr; if (!(dir = opendir(pdom->home))) { virReportSystemError(errno, @@ -339,7 +343,7 @@ static int parallelsFindVmVolumes(virStoragePoolObjPtr pool, goto cleanup; } - while ((ent = readdir(dir)) != NULL) { + while ((direrr = virDirRead(dir, &ent, pdom->home)) > 0) { VIR_FREE(diskPath); VIR_FREE(diskDescPath); @@ -368,8 +372,9 @@ static int parallelsFindVmVolumes(virStoragePoolObjPtr pool, if (parallelsAddDiskVolume(pool, dom, ent->d_name, diskPath, diskDescPath)) goto cleanup; - } + if (direrr < 0) + goto cleanup; ret = 0; cleanup: diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 69a7053..f742dae 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -417,6 +417,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, VIR_DOMAIN_SNAPSHOT_PARSE_INTERNAL); int ret = -1; virCapsPtr caps = NULL; + int direrr; virObjectLock(vm); if (virAsprintf(&snapDir, "%s/%s", baseDir, vm->def->name) < 0) { @@ -439,7 +440,7 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, goto cleanup; } - while ((entry = readdir(dir))) { + while ((direrr = virDirRead(dir, &entry, NULL)) > 0) { if (entry->d_name[0] == '.') continue; @@ -485,6 +486,8 @@ qemuDomainSnapshotLoad(virDomainObjPtr vm, VIR_FREE(fullpath); VIR_FREE(xmlStr); } + if (direrr < 0) + VIR_ERROR(_("Failed to fully read directory %s"), snapDir); if (vm->current_snapshot != current) { VIR_ERROR(_("Too many snapshots claiming to be current for domain %s"), diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c index e9879c4..706db0c 100644 --- a/src/qemu/qemu_hostdev.c +++ b/src/qemu/qemu_hostdev.c @@ -1,7 +1,7 @@ /* * qemu_hostdev.c: QEMU hostdev management * - * Copyright (C) 2006-2007, 2009-2013 Red Hat, Inc. + * Copyright (C) 2006-2007, 2009-2014 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -90,12 +90,13 @@ qemuHostdevHostSupportsPassthroughVFIO(void) DIR *iommuDir = NULL; struct dirent *iommuGroup = NULL; bool ret = false; + int direrr; /* condition 1 - /sys/kernel/iommu_groups/ contains entries */ if (!(iommuDir = opendir("/sys/kernel/iommu_groups/"))) goto cleanup; - while ((iommuGroup = readdir(iommuDir))) { + while ((direrr = virDirRead(iommuDir, &iommuGroup, NULL)) > 0) { /* skip ./ ../ */ if (STRPREFIX(iommuGroup->d_name, ".")) continue; @@ -104,7 +105,7 @@ qemuHostdevHostSupportsPassthroughVFIO(void) break; } - if (!iommuGroup) + if (direrr < 0 || !iommuGroup) goto cleanup; /* okay, iommu is on and recognizes groups */ diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c index a7dfdf0..cd04b20 100644 --- a/src/secret/secret_driver.c +++ b/src/secret/secret_driver.c @@ -1,7 +1,7 @@ /* * secret_driver.c: local driver for secret manipulation API * - * Copyright (C) 2009-2012, 2014 Red Hat, Inc. + * Copyright (C) 2009-2014 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -484,7 +484,7 @@ loadSecrets(virSecretDriverStatePtr driver, driver->directory); goto cleanup; } - while ((de = readdir(dir)) != NULL) { + while (virDirRead(dir, &de, NULL) > 0) { virSecretEntryPtr secret; if (STREQ(de->d_name, ".") || STREQ(de->d_name, "..")) @@ -503,7 +503,7 @@ loadSecrets(virSecretDriverStatePtr driver, } listInsert(&list, secret); } - /* Ignore error reported by readdir(), if any. It's better to keep the + /* Ignore error reported by readdir, if any. It's better to keep the secrets we managed to find. */ while (list != NULL) { diff --git a/src/xen/xen_inotify.c b/src/xen/xen_inotify.c index 7c2d133..cd1e2df 100644 --- a/src/xen/xen_inotify.c +++ b/src/xen/xen_inotify.c @@ -4,7 +4,7 @@ * /etc/xen * /var/lib/xend/domains * - * Copyright (C) 2010-2013 Red Hat, Inc. + * Copyright (C) 2010-2014 Red Hat, Inc. * Copyright (C) 2008 VirtualIron * * This library is free software; you can redistribute it and/or @@ -343,6 +343,7 @@ xenInotifyOpen(virConnectPtr conn, struct dirent *ent; char *path; xenUnifiedPrivatePtr priv = conn->privateData; + int direrr; virCheckFlags(VIR_CONNECT_RO, -1); @@ -363,7 +364,7 @@ xenInotifyOpen(virConnectPtr conn, priv->configDir); return -1; } - while ((ent = readdir(dh))) { + while ((direrr = virDirRead(dh, &ent, priv->configDir)) > 0) { if (STRPREFIX(ent->d_name, ".")) continue; @@ -384,6 +385,8 @@ xenInotifyOpen(virConnectPtr conn, VIR_FREE(path); } closedir(dh); + if (direrr < 0) + return -1; } if ((priv->inotifyFD = inotify_init()) < 0) { diff --git a/src/xen/xm_internal.c b/src/xen/xm_internal.c index f25a7df..30985d8 100644 --- a/src/xen/xm_internal.c +++ b/src/xen/xm_internal.c @@ -327,7 +327,7 @@ xenXMConfigCacheRefresh(virConnectPtr conn) return -1; } - while ((ent = readdir(dh))) { + while ((ret = virDirRead(dh, &ent, priv->configDir)) > 0) { struct stat st; char *path; @@ -386,7 +386,6 @@ xenXMConfigCacheRefresh(virConnectPtr conn) args.now = now; args.priv = priv; virHashRemoveSet(priv->configCache, xenXMConfigReaper, &args); - ret = 0; closedir(dh); -- 1.9.0
More instances of failure to report (unlikely) readdir errors. In one case, I chose to ignore them, given that a readdir error would be no different than timing out on the loop, where the fallback path behaves correctly either way. * src/storage/storage_backend.c (virStorageBackendStablePath): Ignore readdir errors. * src/storage/storage_backend_fs.c (virStorageBackendFileSystemRefresh): Report readdir errors. * src/storage/storage_backend_iscsi.c (virStorageBackendISCSIGetHostNumber): Likewise. * src/storage/storage_backend_scsi.c (getNewStyleBlockDevice) (getBlockDevice, virStorageBackendSCSIFindLUs): Likewise. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/storage/storage_backend.c | 8 +++++--- src/storage/storage_backend_fs.c | 6 ++++-- src/storage/storage_backend_iscsi.c | 5 ++++- src/storage/storage_backend_scsi.c | 10 +++++++--- 4 files changed, 20 insertions(+), 9 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 946196b..58468b3 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1564,6 +1564,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, char *stablepath; int opentries = 0; int retry = 0; + int direrr; /* Short circuit if pool has no target, or if its /dev */ if (pool->def->target.path == NULL || @@ -1604,10 +1605,11 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, * to this device node. * * And it might need some time till the stable path shows - * up, so add timeout to retry here. + * up, so add timeout to retry here. Ignore readdir failures, + * since we have a fallback. */ retry: - while ((dent = readdir(dh)) != NULL) { + while ((direrr = virDirRead(dh, &dent, NULL)) > 0) { if (dent->d_name[0] == '.') continue; @@ -1626,7 +1628,7 @@ virStorageBackendStablePath(virStoragePoolObjPtr pool, VIR_FREE(stablepath); } - if (loop && ++retry < 100) { + if (!direrr && loop && ++retry < 100) { usleep(100 * 1000); goto retry; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 3e3d55f..0f98853 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -854,6 +854,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, struct dirent *ent; struct statvfs sb; virStorageVolDefPtr vol = NULL; + int direrr; if (!(dir = opendir(pool->def->target.path))) { virReportSystemError(errno, @@ -862,7 +863,7 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } - while ((ent = readdir(dir)) != NULL) { + while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) { int ret; char *backingStore; int backingStoreFormat; @@ -924,9 +925,10 @@ virStorageBackendFileSystemRefresh(virConnectPtr conn ATTRIBUTE_UNUSED, if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0) goto cleanup; } + if (direrr < 0) + goto cleanup; closedir(dir); - if (statvfs(pool->def->target.path, &sb) < 0) { virReportSystemError(errno, _("cannot statvfs path '%s'"), diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 881159b..aa6980c 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -93,6 +93,7 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path, int retval = 0; DIR *sysdir = NULL; struct dirent *dirent = NULL; + int direrr; VIR_DEBUG("Finding host number from '%s'", sysfs_path); @@ -107,7 +108,7 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path, goto out; } - while ((dirent = readdir(sysdir))) { + while ((direrr = virDirRead(sysdir, &dirent, sysfs_path)) > 0) { if (STREQLEN(dirent->d_name, "target", strlen("target"))) { if (sscanf(dirent->d_name, "target%u:", host) != 1) { @@ -117,6 +118,8 @@ virStorageBackendISCSIGetHostNumber(const char *sysfs_path, } } } + if (direrr < 0) + retval = -1; closedir(sysdir); out: diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index c448d7f..d037f46 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -237,6 +237,7 @@ getNewStyleBlockDevice(const char *lun_path, DIR *block_dir = NULL; struct dirent *block_dirent = NULL; int retval = 0; + int direrr; if (virAsprintf(&block_path, "%s/block", lun_path) < 0) goto out; @@ -252,7 +253,7 @@ getNewStyleBlockDevice(const char *lun_path, goto out; } - while ((block_dirent = readdir(block_dir))) { + while ((direrr = virDirRead(block_dir, &block_dirent, block_path)) > 0) { if (STREQLEN(block_dirent->d_name, ".", 1)) { continue; @@ -268,6 +269,8 @@ getNewStyleBlockDevice(const char *lun_path, break; } + if (direrr < 0) + retval = -1; closedir(block_dir); @@ -319,6 +322,7 @@ getBlockDevice(uint32_t host, DIR *lun_dir = NULL; struct dirent *lun_dirent = NULL; int retval = 0; + int direrr; if (virAsprintf(&lun_path, "/sys/bus/scsi/devices/%u:%u:%u:%u", host, bus, target, lun) < 0) @@ -333,7 +337,7 @@ getBlockDevice(uint32_t host, goto out; } - while ((lun_dirent = readdir(lun_dir))) { + while ((direrr = virDirRead(lun_dir, &lun_dirent, lun_path)) > 0) { if (STREQLEN(lun_dirent->d_name, "block", 5)) { if (strlen(lun_dirent->d_name) == 5) { retval = getNewStyleBlockDevice(lun_path, @@ -442,7 +446,7 @@ virStorageBackendSCSIFindLUs(virStoragePoolObjPtr pool, snprintf(devicepattern, sizeof(devicepattern), "%u:%%u:%%u:%%u\n", scanhost); - while ((lun_dirent = readdir(devicedir))) { + while ((retval = virDirRead(devicedir, &lun_dirent, device_path)) > 0) { if (sscanf(lun_dirent->d_name, devicepattern, &bus, &target, &lun) != 3) { continue; -- 1.9.0
Now that all clients have been adjusted, ensure that no future misuse of readdir is introduced into the code base. * cfg.mk (sc_prohibit_readdir): New rule. * src/util/virfile.c (virDirRead): Exempt the wrapper. Signed-off-by: Eric Blake <eblake@redhat.com> --- cfg.mk | 6 ++++++ src/util/virfile.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index 8a444d6..3f4bba0 100644 --- a/cfg.mk +++ b/cfg.mk @@ -421,6 +421,12 @@ sc_prohibit_gethostname: halt='use virGetHostname, not gethostname' \ $(_sc_search_regexp) +sc_prohibit_readdir: + @prohibit='\breaddir *\(' \ + exclude='exempt from syntax-check' \ + halt='use virDirRead, not readdir' \ + $(_sc_search_regexp) + sc_prohibit_gettext_noop: @prohibit='gettext_noop *\(' \ halt='use N_, not gettext_noop' \ diff --git a/src/util/virfile.c b/src/util/virfile.c index bb1e7ba..f75f0c8 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2401,7 +2401,7 @@ virDirCreate(const char *path ATTRIBUTE_UNUSED, int virDirRead(DIR *dirp, struct dirent **ent, const char *name) { errno = 0; - *ent = readdir(dirp); + *ent = readdir(dirp); /* exempt from syntax-check */ if (!*ent && errno) { if (name) virReportSystemError(errno, _("Unable to read directory '%s'"), -- 1.9.0
On 04/26/2014 06:14 AM, Eric Blake wrote:
This extends the work started by Nataneal to cover the entire tree. Basically, readdir() is awkward to use correctly: to properly check for (unlikely) errors, one must pre-set errno. Forcing everything to go through a wrapper ensures that all our uses will be consistent; this also fixes several bugs in some of the call sites.
Eric Blake (5): conf: use virDirRead API util: use virDirRead API drivers: use virDirRead API storage: use virDirRead API enforce sane readdir usage
Natanael Copa (2): util: introduce virDirRead wrapper for readdir nodeinfo: use virDirRead API
cfg.mk | 6 +++ src/conf/domain_conf.c | 5 ++- src/conf/network_conf.c | 11 +++--- src/conf/nwfilter_conf.c | 6 +-- src/conf/storage_conf.c | 6 +-- src/libvirt_private.syms | 1 + src/nodeinfo.c | 31 +++++---------- src/openvz/openvz_conf.c | 9 +---- src/parallels/parallels_storage.c | 11 ++++-- src/qemu/qemu_driver.c | 5 ++- src/qemu/qemu_hostdev.c | 7 ++-- src/secret/secret_driver.c | 6 +-- src/storage/storage_backend.c | 8 ++-- src/storage/storage_backend_fs.c | 6 ++- src/storage/storage_backend_iscsi.c | 5 ++- src/storage/storage_backend_scsi.c | 10 +++-- src/util/vircgroup.c | 28 ++++++++------ src/util/virfile.c | 76 +++++++++++++++++++++++-------------- src/util/virfile.h | 4 ++ src/util/virnetdevtap.c | 10 +---- src/util/virpci.c | 20 +++++----- src/util/virscsi.c | 9 +++-- src/util/virusb.c | 7 +++- src/util/virutil.c | 4 +- src/xen/xen_inotify.c | 7 +++- src/xen/xm_internal.c | 3 +- 26 files changed, 170 insertions(+), 131 deletions(-)
ACK series if you conver the newly addded readdir call as well: src/network/bridge_driver.c:487: entry = readdir(dir); maint.mk: use virDirRead, not readdir make: *** [sc_prohibit_readdir] Error 1 Jan
On 04/28/2014 12:23 PM, Ján Tomko wrote:
On 04/26/2014 06:14 AM, Eric Blake wrote:
This extends the work started by Nataneal to cover the entire tree. Basically, readdir() is awkward to use correctly: to properly check for (unlikely) errors, one must pre-set errno. Forcing everything to go through a wrapper ensures that all our uses will be consistent; this also fixes several bugs in some of the call sites.
Eric Blake (5): conf: use virDirRead API util: use virDirRead API drivers: use virDirRead API storage: use virDirRead API enforce sane readdir usage
Natanael Copa (2): util: introduce virDirRead wrapper for readdir nodeinfo: use virDirRead API
cfg.mk | 6 +++ src/conf/domain_conf.c | 5 ++- src/conf/network_conf.c | 11 +++--- src/conf/nwfilter_conf.c | 6 +-- src/conf/storage_conf.c | 6 +-- src/libvirt_private.syms | 1 + src/nodeinfo.c | 31 +++++---------- src/openvz/openvz_conf.c | 9 +---- src/parallels/parallels_storage.c | 11 ++++-- src/qemu/qemu_driver.c | 5 ++- src/qemu/qemu_hostdev.c | 7 ++-- src/secret/secret_driver.c | 6 +-- src/storage/storage_backend.c | 8 ++-- src/storage/storage_backend_fs.c | 6 ++- src/storage/storage_backend_iscsi.c | 5 ++- src/storage/storage_backend_scsi.c | 10 +++-- src/util/vircgroup.c | 28 ++++++++------ src/util/virfile.c | 76 +++++++++++++++++++++++-------------- src/util/virfile.h | 4 ++ src/util/virnetdevtap.c | 10 +---- src/util/virpci.c | 20 +++++----- src/util/virscsi.c | 9 +++-- src/util/virusb.c | 7 +++- src/util/virutil.c | 4 +- src/xen/xen_inotify.c | 7 +++- src/xen/xm_internal.c | 3 +- 26 files changed, 170 insertions(+), 131 deletions(-)
ACK series if you conver the newly addded readdir call as well: src/network/bridge_driver.c:487: entry = readdir(dir); maint.mk: use virDirRead, not readdir make: *** [sc_prohibit_readdir] Error 1
To make it easier for Eric (and since it was me that added the new usage of readdir) I just sent a "PATCHv2 6.5/7" to this thread that converts to virDirRead().
On 04/28/2014 03:23 AM, Ján Tomko wrote:
On 04/26/2014 06:14 AM, Eric Blake wrote:
This extends the work started by Nataneal to cover the entire tree. Basically, readdir() is awkward to use correctly: to properly check for (unlikely) errors, one must pre-set errno. Forcing everything to go through a wrapper ensures that all our uses will be consistent; this also fixes several bugs in some of the call sites.
Eric Blake (5): conf: use virDirRead API util: use virDirRead API drivers: use virDirRead API storage: use virDirRead API enforce sane readdir usage
Natanael Copa (2): util: introduce virDirRead wrapper for readdir nodeinfo: use virDirRead API
ACK series if you conver the newly addded readdir call as well: src/network/bridge_driver.c:487: entry = readdir(dir); maint.mk: use virDirRead, not readdir make: *** [sc_prohibit_readdir] Error 1
I've pushed the series, including Laine's addition. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
This attempts to follow the same variable name and usage patterns as the other conversions to virDirRead(). --- src/network/bridge_driver.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 4e8375d..5233833 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -463,6 +463,7 @@ networkMigrateStateFiles(virNetworkDriverStatePtr driver) int ret = -1; const char *oldStateDir = LOCALSTATEDIR "/lib/libvirt/network"; DIR *dir; + int direrr; struct dirent *entry; char *oldPath = NULL, *newPath = NULL; char *contents = NULL; @@ -482,17 +483,7 @@ networkMigrateStateFiles(virNetworkDriverStatePtr driver) goto cleanup; } - for (;;) { - errno = 0; - entry = readdir(dir); - if (!entry) { - if (errno) { - virReportSystemError(errno, _("failed to read directory '%s'"), - oldStateDir); - goto cleanup; - } - break; - } + while ((direrr = virDirRead(dir, &entry, oldStateDir)) > 0) { if (entry->d_type != DT_REG || STREQ(entry->d_name, ".") || @@ -520,6 +511,8 @@ networkMigrateStateFiles(virNetworkDriverStatePtr driver) VIR_FREE(newPath); VIR_FREE(contents); } + if (direrr < 0) + goto cleanup; ret = 0; cleanup: -- 1.9.0
On 04/28/2014 12:45 PM, Laine Stump wrote:
This attempts to follow the same variable name and usage patterns as the other conversions to virDirRead(). --- src/network/bridge_driver.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-)
ACK Jan
participants (3)
-
Eric Blake -
Ján Tomko -
Laine Stump