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(a)redhat.com&gt; --- 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

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(a)redhat.com&gt; --- 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