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

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(a)redhat.com&gt; wrote: >> >> On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau(a)redhat.com wrote: >> >From: Marc-André Lureau <marcandre.lureau(a)redhat.com&gt; >> > >> >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(a)redhat.com&gt; >> >--- >> > 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(a)redhat.com&gt; > >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 &gt;libvir-list(a)redhat.com >https://www.redhat.com/mailman/listinfo/libvir-list