Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#1317 closed defect (fixed)

check for Rainbow binaries before trying to use them

Reported by: sascha_silbe Owned by: sascha_silbe
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: Git as of bugdate
Severity: Unspecified Keywords: r+
Cc: m_stone, tomeu Distribution/OS: Unspecified
Bug Status: Assigned

Description

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.

Attachments (2)

0001-don-t-use-rainbow-if-it-has-been-uninstalled-but-the.patch (1.2 KB) - added by sascha_silbe 11 years ago.
don't use rainbow if it has been uninstalled, but the config file remains
sugar-toolkit-1317.patch (1.3 KB) - added by sascha_silbe 11 years ago.
don't use rainbow if it has been uninstalled, but the config file remains

Download all attachments as: .zip

Change History (20)

comment:1 Changed 11 years ago by sascha_silbe

  • Keywords r? added

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

comment:2 Changed 11 years ago by sascha_silbe

  • Component changed from sugar to sugar-toolkit
  • Status changed from new to accepted

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

comment:3 Changed 11 years ago by tomeu

  • Cc m_stone added

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

Would be great if Michael could give it a look.

comment:4 follow-up: Changed 11 years ago by 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.

comment:5 in reply to: ↑ 4 Changed 11 years ago by 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.

comment:6 Changed 11 years ago by erikos

  • Keywords r! added; r? removed

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

Changed 11 years ago by sascha_silbe

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

comment:7 Changed 11 years ago by sascha_silbe

  • Keywords r? added; r! removed

Patch updated.

comment:8 follow-up: Changed 11 years ago by tomeu

  • Keywords r! added; r? removed
 	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.

Changed 11 years ago by sascha_silbe

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

comment:9 in reply to: ↑ 8 Changed 11 years ago by sascha_silbe

  • Keywords r? added; r! removed

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.

comment:10 Changed 11 years ago by sascha_silbe

Is this ready to go into 0.87 now?

comment:11 Changed 11 years ago by sascha_silbe

  • Cc tomeu added

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

comment:12 Changed 11 years ago by tomeu

  • Component changed from sugar-toolkit to sugar-presence-service

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.

comment:13 Changed 11 years ago by tomeu

  • Keywords r! added; r? removed

ping

comment:14 Changed 11 years ago by sascha_silbe

  • Keywords r? added; r! removed

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).

comment:15 Changed 11 years ago by tomeu

  • Keywords r+ added; r? removed

Please push, thanks!

comment:16 Changed 11 years ago by sascha_silbe

  • Component changed from sugar-presence-service to sugar-toolkit
  • Resolution set to fixed
  • Status changed from accepted to closed

Pushed as 1d8b1b9, thanks!

comment:17 Changed 8 years ago by dnarvaez

  • Component changed from sugar-toolkit to Sugar

comment:18 Changed 8 years ago by dnarvaez

  • Milestone 0.86 deleted

Milestone 0.86 deleted

Note: See TracTickets for help on using tickets.