Re: [Kimchi-devel] [PATCH v2] Set virt_use_nfs when NFS pool is added.

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(a)linux.vnet.ibm.com&gt; >> --- >> 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?

Regards, - Christy