[libvirt] [PATCH] sanlock: Enhance error message to point to possible problem with selinux

[Daniel P. Berrange]

On Thu, Mar 15, 2012 at 10:55:20AM -0600, Eric Blake wrote:
> On 03/15/2012 10:35 AM, Daniel P. Berrange wrote:
> > On Thu, Mar 15, 2012 at 05:23:09PM +0100, Peter Krempa wrote:
> >> If the connection to the sanlock daemon is forbidden by selinux the
> >> error message was not clear enough. This patch adds a check if proper
> >> configuration for selinux is used while trying to connect to sanlock.
> >>
> >> *src/locking/lock_driver_sanlock.c:
> >>         - add macro virLockSystemError that checks for selinux and
> >>           reports an improved error message
> >>         - modify calls of virReportSystemError to the new macro in
> >>           apropriate places
> >>
> >> Background:
> >> https://bugzilla.redhat.com/show_bug.cgi?id=770488
> > 
> > IMHO this is not something we should do here. You're outputing the
> > message regardless of whether there is even an NFS volume involved,
> > and harcoding details of the SELinux policy. Finally I don't think
> > we should blindly tell people to change SELinux tunables without
> > explaining the implications, which is not practical in an error
> > message.
> 
> We've done this sort of targeted error message before; but there we were
> careful to _only_ issue the message after checking that we were indeed
> dealing with NFS; see commit 1888363d.  [Hmm - should that patch be
> revisited, to mention virt_use_samba if it was samba rather than nfs
> that caused the SELinux denial, since those are different bools?]

I don't really agree with that previous commit either for the
same reasons.

> 
> > 
> > So, IMHO, this belongs in documentation, not in the error messages
> > here.
> 
> I think both are appropriate - we definitely need to mention
> virt_use_sanlock in locking.html.in, with full implications, but I would
> also like to see the error message, provided that you can be sure that
> the error message only mentions virt_use_sanlock in the actual case
> where SELinux is enforcing and the error is confirmed to be due to NFS
> causing us to need the bool.

I still don't think it belongs in the error message - with this
kind of message, they'll just see the instruction & blindly follow it

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|