[libvirt] [PATCH v4 02/20] util: ignore EACCESS in virDirOpenIfExists

[Cole Robinson]

On 9/13/19 8:50 AM, marcandre.lureau at redhat.com wrote:
> From: Marc-André Lureau <marcandre.lureau at redhat.com>
> 
> Whether a directory exists or is not readable shouldn't make a big
> diffence.
> 
> This removes errors when firmare or vhost-user config is looked up
> from a user session libvirt if /etc/libvirt is not readable.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> ---
>   src/util/virfile.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 

In some cases this makes sense, in others it isn't clear. For example:

[:~] $ chmod 000 .config/libvirt/storage/
[:~] $ libvirtd
...
2019-09-20 19:45:13.691+0000: 327223: error : virDirOpenInternal:2846 : 
cannot open directory '/home/crobinso/.config/libvirt/storage': 
Permission denied
2019-09-20 19:45:13.691+0000: 327223: error : virStateInitialize:656 : 
Initialization of storage state driver failed: cannot open directory 
'/home/crobinso/.config/libvirt/storage': Permission denied
2019-09-20 19:45:13.691+0000: 327223: error : daemonRunStateInit:790 : 
Driver state initialization failed

After the patches, this case doesn't explicitly fail, but the driver 
won't report any existing storage pools so it causes silent issues. I 
think erroring incase permissions are wrong is better, because libvirt 
doesn't have any code to attempt to fix that situation, unlike for 
missing directory

Not sure if it's realistic for this case to happen but I noticed it 
through code inspection.

Since this only applies to your particular case in the code in one area, 
you can do (untested)

     if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) {
         /* descriptive comment */
         if (virLastErrorIsSystemErrno(EACCES)) {
             virResetLastError();
             return 0;
         }
         return -1;
     }


> diff --git a/src/util/virfile.c b/src/util/virfile.c
> index bb844c64e5..4d1fe50efc 100644
> --- a/src/util/virfile.c
> +++ b/src/util/virfile.c
> @@ -2855,7 +2855,8 @@ virFileRemove(const char *path,
>   #endif /* WIN32 */
>   
>   static int
> -virDirOpenInternal(DIR **dirp, const char *name, bool ignoreENOENT, bool quiet)
> +virDirOpenInternal(DIR **dirp, const char *name,
> +                   bool ignoreENOENT, bool ignoreEACCESS, bool quiet)
>   {
>       *dirp = opendir(name); /* exempt from syntax-check */
>       if (!*dirp) {
> @@ -2864,6 +2865,8 @@ virDirOpenInternal(DIR **dirp, const char *name, bool ignoreENOENT, bool quiet)
>   
>           if (ignoreENOENT && errno == ENOENT)
>               return 0;
> +        if (ignoreEACCESS && errno == EACCES)
> +            return 0;
>           virReportSystemError(errno, _("cannot open directory '%s'"), name);
>           return -1;
>       }
> @@ -2881,7 +2884,7 @@ virDirOpenInternal(DIR **dirp, const char *name, bool ignoreENOENT, bool quiet)
>   int
>   virDirOpen(DIR **dirp, const char *name)
>   {
> -    return virDirOpenInternal(dirp, name, false, false);
> +    return virDirOpenInternal(dirp, name, false, false, false);
>   }
>   
>   /**
> @@ -2890,13 +2893,13 @@ virDirOpen(DIR **dirp, const char *name)
>    * @name: path of the directory
>    *
>    * Returns 1 on success.
> - * If opendir returns ENOENT, 0 is returned without reporting an error.
> + * If opendir returns ENOENT or EACCES, 0 is returned without reporting an error.
>    * On other errors, -1 is returned and an error is reported.
>    */
>   int
>   virDirOpenIfExists(DIR **dirp, const char *name)
>   {
> -    return virDirOpenInternal(dirp, name, true, false);
> +    return virDirOpenInternal(dirp, name, true, true, false);
>   }
>   
>   /**
> @@ -2912,7 +2915,7 @@ virDirOpenIfExists(DIR **dirp, const char *name)
>   int
>   virDirOpenQuiet(DIR **dirp, const char *name)
>   {
> -    return virDirOpenInternal(dirp, name, false, true);
> +    return virDirOpenInternal(dirp, name, false, false, true);
>   }
>   
>   /**
> 


- Cole