1. Patchew
  2. Libvirt
  3. [libvirt] [PATCH v5 00/20] Hi,
  4. View patch

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

marcandre.lureau@redhat.com posted 20 patches 6 years, 4 months ago
[libvirt] [PATCH v5 02/20] qemu-interop: ignore non-readable directories
Posted by marcandre.lureau@redhat.com 6 years, 4 months ago 
From: Marc-André Lureau <marcandre.lureau@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@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 */
+        if (virLastErrorIsSystemErrno(EACCES)) {
+            virResetLastError();
+            return 0;
+        }
         return -1;
+    }
 
     if (rc == 0)
         return 0;
-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 02/20] qemu-interop: ignore non-readable directories
Posted by Ján Tomko 6 years, 4 months ago 
On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau@redhat.com wrote:
>From: Marc-André Lureau <marcandre.lureau@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@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.

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.

>+        if (virLastErrorIsSystemErrno(EACCES)) {
>+            virResetLastError();
>+            return 0;
>+        }
>         return -1;
>+    }
>

With that fixed:
Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 02/20] qemu-interop: ignore non-readable directories
Posted by Marc-André Lureau 6 years, 4 months ago 
Hi

On Mon, Sep 23, 2019 at 4:35 PM Ján Tomko <jtomko@redhat.com> wrote:
>
> On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau@redhat.com wrote:
> >From: Marc-André Lureau <marcandre.lureau@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@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@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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 02/20] qemu-interop: ignore non-readable directories
Posted by Ján Tomko 6 years, 4 months ago 
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@redhat.com> wrote:
>>
>> On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau@redhat.com wrote:
>> >From: Marc-André Lureau <marcandre.lureau@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@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@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@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5 02/20] qemu-interop: ignore non-readable directories
Posted by Marc-André Lureau 6 years, 4 months ago 
Hi

On Tue, Sep 24, 2019 at 2:08 PM Ján Tomko <jtomko@redhat.com> wrote:
>
> 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@redhat.com> wrote:
> >>
> >> On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau@redhat.com wrote:
> >> >From: Marc-André Lureau <marcandre.lureau@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@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@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.
>

It's treated as an error: it logs an error, but it's not fatal.

> 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

It could be a misconfiguration or an administrator policy. In both
cases, it's not a good idea to prevent the user from starting the VM
with the user configs.

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

Sounds okay to me, but the user would have no clear clue that
something unusual happened (that system config are not readable).

>
> Jano
>
>
> >thanks
> >
> >--
> >libvir-list mailing list
> >libvir-list@redhat.com
> >https://www.redhat.com/mailman/listinfo/libvir-list

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.