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");