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@xxxxxxxxxx> wrote:On Mon, Sep 23, 2019 at 02:44:25PM +0400, marcandre.lureau@xxxxxxxxxx wrote: >From: Marc-André Lureau <marcandre.lureau@xxxxxxxxxx> > >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@xxxxxxxxxx> >--- > 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@xxxxxxxxxx>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@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
