check for Rainbow binaries before trying to use them

[sascha_silbe]

Jonas Smedegaard on sugar-devel:

It seems to me that the code currently does not check if rainbow is installed, only if some rainbow config space exist - which seems wrong to me.

and:

If that is correct, then I recommend changing that to instead test for the existence of some binary, as the Debian packaging system (and most probably other packaging systems as well) preserve configuration files when removing a package.

[sascha_silbe]

I've tested this patch with
a) Rainbow never installed (neither rainbow-run nor /etc/olpc-security exist)
b) Rainbow active (both exist)
c) Rainbow inactive (rainbow-run exists, but not /etc/olpc-security)
d) Rainbow never installed, but /etc/olpc-security exists

[sascha_silbe]

|TestCase|

1. Ensure Rainbow is not installed.
2. Create the file /etc/olpc-security
3. Try running any activity (e.g. Calculate). It should not fail.

[tomeu]

Why is self._rainbow_enabled a class member instead of a local var?

Would be great if Michael could give it a look.

[sascha_silbe]

_rainbow_enabled is an instance (not class) attribute because it executes a binary (didn't want to add a function to replicate exec*p() behaviour), so is quite expensive. Storing a boolean is quite cheap OTOH.

[tomeu]

Replying to sascha_silbe:

_rainbow_enabled is an instance (not class) attribute because it executes a binary (didn't want to add a function to replicate exec*p() behaviour), so is quite expensive. Storing a boolean is quite cheap OTOH.

That class seems to be instantiated every time we want to launch an activity, so you aren't saving any exec calls.

[erikos]

Ok, as Tomeu suggested, please use a local var.

command = ['/usr/bin/sudo', '-E', '--', 
	 	                       '/usr/bin/rainbow-run
command = ['sudo', '-E', '--', 
 		                       'rainbow-run', 

This is not really part of fixing the problem I presume. Agreed that it might be worth to do so, but please use another patch for this and state about it's importance.

Thanks

[sascha_silbe]

don't use rainbow if it has been uninstalled, but the config file remains

[sascha_silbe]

Patch updated.

[tomeu]

 	247	        dev_null = file('/dev/null', 'w')

Won't be leaking descriptors like this?

 	249	        rainbow_enabled = subprocess.call(['which', 'rainbow-run'], 
 	250	            stdout=dev_null, stderr=dev_null) == 0 and \ 
 	251	            os.path.exists('/etc/olpc-security') 

I think that it's better to try to have one statement per line, rather than cramming several stuff in just one statement. Intermediate variables are a chance to communicate what your code is doing without having to use comments or letting the reader guess it.

[sascha_silbe]

don't use rainbow if it has been uninstalled, but the config file remains

[sascha_silbe]

Replying to tomeu:

 	247	        dev_null = file('/dev/null', 'w')

Won't be leaking descriptors like this?

Nop, Python garbage collection should take care of it.

I think that it's better to try to have one statement per line, rather than cramming several stuff in just one statement. Intermediate variables are a chance to communicate what your code is doing without having to use comments or letting the reader guess it.

New patch attached.

[sascha_silbe]

Is this ready to go into 0.87 now?

[sascha_silbe]

Maybe I should add tomeu to CC if I want to get his attention. ;)

[tomeu]

Patch looks good to me, but could you check on a XO-1 if it makes activity startup any slower? It's already slow enough.

[tomeu]

ping

[sascha_silbe]

Timed on my XO-1 running from nfs-root (most likely slower than running from the internal NAND):

1262959135.648757 DEBUG root: pre rainbow check: 1262959135.648518
1262959135.774819 DEBUG root: post rainbow check: 1262959135.774293

I.e. about 130ms for the entire check (first run).

[tomeu]

Please push, thanks!

[sascha_silbe]

Pushed as ​1d8b1b9, thanks!

[dnarvaez]

Milestone 0.86 deleted