Re: [libvirt] [PATCH 06/18] Use virDirOpen

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...).

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.

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.

> > 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.

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