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