[libvirt] [PATCH v5 02/20] qemu-interop: ignore non-readable directories

[Marc-André Lureau]

Hi

On Mon, Sep 23, 2019 at 4:35 PM Ján Tomko <jtomko at redhat.com> wrote:
>
> On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau at redhat.com wrote:
> >From: Marc-André Lureau <marcandre.lureau at redhat.com>
> >
> >Whether a directory is missing or is not readable doesn't make much
> >difference when populating the available configs.
> >
> >This removes errors when firmare or vhost-user config is looked up
> >from a user session libvirt if /etc/libvirt is not readable for ex.
> >
> >Signed-off-by: Marc-André Lureau <marcandre.lureau at redhat.com>
> >---
> > src/qemu/qemu_interop_config.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> >diff --git a/src/qemu/qemu_interop_config.c b/src/qemu/qemu_interop_config.c
> >index 1f39d4b576..f3c5d2e083 100644
> >--- a/src/qemu/qemu_interop_config.c
> >+++ b/src/qemu/qemu_interop_config.c
> >@@ -41,8 +41,14 @@ qemuBuildFileList(virHashTablePtr files, const char *dir)
> >     int rc;
> >     int ret = -1;
> >
> >-    if ((rc = virDirOpenIfExists(&dirp, dir)) < 0)
> >+    if ((rc = virDirOpenIfExists(&dirp, dir)) < 0) {
> >+        /* silently ignore unreadable directories */
>
> This will not be silent - virDirOpenIfExists already logged an error.

Right, let's drop the comment.

>
> You can call virFileExists upfront - which is just a wrapper to access
> and then take virDirOpenIfExists errors seriously - if someone changes
> the permissions in the meantime, they deserve the error.

That won't work the desired way, since virFileExists() is true even
when you don't have permissions to read it.

>
> >+        if (virLastErrorIsSystemErrno(EACCES)) {
> >+            virResetLastError();
> >+            return 0;
> >+        }
> >         return -1;
> >+    }
> >
>
> With that fixed:
> Reviewed-by: Ján Tomko <jtomko at redhat.com>

I see several ways forward, if any of the directory doesn't have
readable permissions :

1. keep behaviour before this patch: fail entirely. This is
unnecessarily strict imho
2. use this patch (remove the comment): log an error, but browse other
directories
3. change the code further so that no error is logged

To me, 2 is a good compromise.

Does your r-b tag holds with the comment removed only?

thanks