[libvirt] [PATCH 06/18] Use virDirOpen

[John Ferlan]

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