Thanks Aline! On Mon, 2014-04-14 at 15:59 -0300, Aline Manera wrote:
On 04/11/2014 02:06 PM, Christy Perez wrote:
On Fri, 2014-04-11 at 11:54 +0800, Mark Wu wrote:
On 04/11/2014 06:56 AM, Christy Perez wrote:
selinux has a special boolean to make it easier for disk images to be managedi by libvirt. Set this to true when a user adds an NFS storage pool.
Most virtualzation documentation recommends that this be set to true. For example: http://www.ovirt.org/Troubleshooting_NFS_Storage_Issues http://fedoraproject.org/wiki/How_to_debug_Virtualization_problems
This will leave it set to true, even if the user removes NFS storage pools. It is not a security risk, and we should not set it to False in case it had already been set by the user for another non-kimchi use.
Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> --- src/kimchi/model/storagepools.py | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py index 5af33b7..1ec6e99 100644 --- a/src/kimchi/model/storagepools.py +++ b/src/kimchi/model/storagepools.py @@ -126,6 +126,13 @@ class StoragePoolsModel(object): kimchi_log.error("Problem creating Storage Pool: %s", e) raise OperationFailed("KCHPOOL0007E", {'name': name, 'err': e.get_error_message()}) + if params['type'] == 'netfs': + output, error, returncode = run_command(['setsebool', '-P', + 'virt_use_nfs=1']) The persistent change of sebool is a very time-consuming operation compared with the runtime change. Please see: [root@localhost libvirt]# time setsebool -P virt_use_nfs=1
real 0m10.686s user 0m9.680s sys 0m0.337s [root@localhost libvirt]# time setsebool virt_use_nfs=1
real 0m0.035s user 0m0.001s sys 0m0.005s
time getsebool virt_use_nfs virt_use_nfs --> off
real 0m0.002s user 0m0.000s sys 0m0.001s
So I think we could have a little optimization here: check the bool value by getsebool before setting it. I am fine to leave it in a following up patch.
+ if error or returncode: Just checking returncode is enough, isn't it? + kimchi_log.error('Unable to set virt_use_nfs=1. If you use + SELinux, this may prevent NFS pools from + being used.') return name
def _clean_scan(self, pool_name): Thanks for the comments, Mark. This has uncovered some interesting behavior in trying to implement your suggestion.
The error situation I was trying to handle here is if (as Royce pointed out), there is no selinux on a system. I assumed that if the command wasn't found, there wouldn't be an rc, but that an error message would come back. I did some experiments, and it turns out that the way kimchi's run_command() works, if a command doesn't exist, there's an error printed (e.g. Failed to run command: fakesebool -P virt_use_nfs=1. [Errno 2] No such file or directory), but then run_command returns None, None, None because of the exception. So, really, I'll get back no error or returncode in this situation! I'm not sure this is the correct behavior in case of an exception -- but that can be taken up in a separate thread.
This also makes it more difficult to know if getsebool virt_use_nfs is set or not. If getsebool causes that exception, it won't get back an rc, or output, or an error message.
Since this won't be called very often, I think it's okay to leave it as-is (except fix the errors that Aline pointed out in my string), and revisit after we decide how to deal with run_command().
If that sounds okay, Aline -- are you ok with throwing in some new-line chars at the end of those strings, or do you want me to submit a new patch with that?
I will fix it before applying so you don't need to resend the patch.
Regards,
- Christy