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

[Ján Tomko]

On Tue, Sep 24, 2019 at 10:12:19AM +0400, Marc-André Lureau wrote:
>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?
>

Logging errors that are not treated as such pollutes logs and misleads
users.

I assume the only possible path here is QEMU_ETC_LOCATION - the user
should have access to $HOME and preventing access to /usr/share does
not sound like a reasonable system.

What would be the benefits of making /etc/qemu private? If it's
a misconfiguration, we don't have to be lenient here and can go with #1

Otherwise, making
  virFileAccessibleAs(etcLocation, R_OK | X_OK, geteuid(), getegid())
a condition for
    if (qemuBuildFileList(files, etcLocation) < 0)
        return -1;
seems to be the least effort to silence unaccessible /etc/qemu while still
erroring out on the required cases.

Jano


>thanks
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20190924/680218dc/attachment-0001.sig>