On Thu, Jun 23, 2016 at 08:26:30AM -0400, John Ferlan wrote:
On 06/21/2016 12:05 PM, Ján Tomko wrote:
Switch from opendir to virDirOpen everywhere we need to report an error.
---
src/storage/storage_backend_fs.c | 6 +-----
src/storage/storage_backend_iscsi.c | 7 +------
src/storage/storage_backend_scsi.c | 19 +++----------------
src/util/vircgroup.c | 5 +----
src/util/virfile.c | 16 +++-------------
src/util/virhostcpu.c | 4 +---
src/util/virnetdev.c | 6 +-----
src/util/virnetdevtap.c | 7 +------
src/util/virpci.c | 13 +++----------
src/util/virprocess.c | 2 +-
src/util/virscsi.c | 10 ++--------
src/util/virusb.c | 7 +------
src/util/virutil.c | 18 +++---------------
src/xen/xen_inotify.c | 7 ++-----
src/xen/xm_internal.c | 6 +-----
tests/virschematest.c | 6 +-----
tools/nss/libvirt_nss.c | 4 +---
17 files changed, 27 insertions(+), 116 deletions(-)
There were 4 in this pile which it's not 100% clear if they should
actually be using the event virDirOpenQuiet...
[...]
diff --git a/src/util/virpci.c b/src/util/virpci.c
index 5cb5d3a..77ae9b4 100644
--- a/src/util/virpci.c
+++ b/src/util/virpci.c
@@ -460,11 +460,8 @@ virPCIDeviceIterDevices(virPCIDeviceIterPredicate predicate,
VIR_DEBUG("%s %s: iterating over " PCI_SYSFS "devices", dev->id, dev->name);
- dir = opendir(PCI_SYSFS "devices");
- if (!dir) {
- VIR_WARN("Failed to open " PCI_SYSFS "devices");
+ if (virDirOpen(&dir, PCI_SYSFS "devices") < 0)
return -1;
- }
This one it seems eventually would cause an error (although on opendir
failure, the failure to open message could be issued twice...).
Both of the other error paths (virDirRead and virPCIDeviceNew) report an
error, so opendir should be noisy too.
Also, failing to open /sys/bus/pci/devices is unlikely.
It seems
virPCIDeviceBusContainsActiveDevices may not want the error since it's
caller virPCIDeviceTrySecondaryBusReset on failure will call
virPCIDeviceGetParent which would seem to want the error.
If virPCIDeviceBusContainsActiveDevices fails and returns -1 (which is
unlikely - either we failed to opendir /sys/bus/pci/devices, or we got
EBADF from readdir on that same DIR*, or OOM), then
if ((conflict = virPCIDeviceBusContainsActiveDevices(dev,
inactiveDevs))) {
is true and it exits early. I do not see the virPCIDeviceGetParent call.
In this
situation, I believe we'll see two failure to open errors.
while ((ret = virDirRead(dir, &entry, PCI_SYSFS "devices")) > 0) {
unsigned int domain, bus, slot, function;
@@ -1962,11 +1959,8 @@ int virPCIDeviceFileIterate(virPCIDevicePtr dev,
dev->address.slot, dev->address.function) < 0)
goto cleanup;
- if (!(dir = opendir(pcidir))) {
- virReportSystemError(errno,
- _("cannot open %s"), pcidir);
+ if (virDirOpen(&dir, pcidir) < 0)
goto cleanup;
- }
while ((direrr = virDirRead(dir, &ent, pcidir)) > 0) {
/* Device assignment requires:
@@ -2696,8 +2690,7 @@ virPCIGetNetName(char *device_link_sysfs_path, char **netname)
return -1;
}
- dir = opendir(pcidev_sysfs_net_path);
- if (dir == NULL)
+ if (virDirOpen(&dir, pcidev_sysfs_net_path) < 0)
goto out;
Quite a few callers - I didn't chase them all, but it's whether they all
expect to have an error is a matter of interpretation. The callers (PF
and VF searches) seem to have varying opinions on whether a < 0 return
is really an error.
I'll change this one to use the Quiet version.
while (virDirRead(dir, &entry, pcidev_sysfs_net_path) > 0) {
diff --git a/src/util/virprocess.c b/src/util/virprocess.c
index b0ca1ce..a3e7f4e 100644
--- a/src/util/virprocess.c
+++ b/src/util/virprocess.c
@@ -612,7 +612,7 @@ int virProcessGetPids(pid_t pid, size_t *npids, pid_t **pids)
(unsigned long long)pid) < 0)
goto cleanup;
- if (!(dir = opendir(taskPath)))
+ if (virDirOpen(&dir, taskPath) < 0)
goto cleanup;
Another, although perhaps it's good in this case.
while ((value = virDirRead(dir, &ent, taskPath)) > 0) {
[...]
diff --git a/tools/nss/libvirt_nss.c b/tools/nss/libvirt_nss.c
index d179514..724cb06 100644
--- a/tools/nss/libvirt_nss.c
+++ b/tools/nss/libvirt_nss.c
@@ -125,10 +125,8 @@ findLease(const char *name,
}
- if (!(dir = opendir(leaseDir))) {
- ERROR("Failed to open dir '%s'", leaseDir);
+ if (virDirOpen(&dir, leaseDir) < 0)
goto cleanup;
- }
For this one ERROR is eaten by the # define; however, if ERROR was
defined, then I don't believe ERROR would process things properly.
The error handling is a bit strange in that file, but it also uses
VIR_REALLOC_QUIET, so I'll change this one too.
Jan
Conditional ACK - I would think virOpenDirQuiet could be the better
choice for these though
John
if (!(leases_array = virJSONValueNewArray())) {
ERROR("Failed to create json array");
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list