[tor-dev] DetecTor Review

[Damian Johnson]

Hi Kai. I just took a quick peek at your DetecTor project
(http://detector.io/). It looks like the project has been inactive for
a while so just some quick thoughts in case it sparks back up.


> log_setting = "notice file " + data_dir + os.sep + "tor.log"

The following might be nicer...

  log_setting = "notice file %s%stor.log" % (data_dir, os.sep)

Same goes for most instances of string concatenation, actually.

> 'StrictExitNodes': str(1),

Interesting, never seen this before. Should be the following instead.

  'StrictExitNodes': '1',

> def stop_tor(control_port):
>     tor_pid = stem.util.system.get_pid_by_port(control_port)
>     if tor_pid:
>         os.kill(tor_pid, signal.SIGTERM)

Certainly a fine approach. Would you like to avoid having Tor outlive
this python process? If so then that's as easy as including
'take_ownership = True' in your launch_tor_with_config() call. If not
then you can use the process object returned by
launch_tor_with_config() to kill it as well...

  tor_process = stem.process.launch_tor_with_config()
  ... do stuff...
  tor_process.kill()

Also this method has tabs mixed with spaces. I would highly recommend
strictly using spaces - python is whitespace dependent, and occasional
tabs can make things a lot more confusing.

> def is_tor_running(control_port):
>     return bool(stem.util.system.get_pid_by_port(control_port))

I believe you can use the poll() method of the process object to make
this a little less platform dependent.

> if os.path.isdir(dir_name) != True:

You never need to compare with booleans in an if statement. This is
equivalent to...

  if not os.path.isdir(dir_name):

> config.readfp(open('sphere_control.config'))

Minor thing but this doesn't properly close the file handle. It's
probably done on its own during GC, but it would be more proper to
do...

  with open('sphere_control.config') as config_file:
    config.readfp(config_file)

Honestly I'm really not sure what the following code is doing. I
suspect there's a lot of low hanging fruit for improving this (for
instance using "', '.join(countries_list[i])" instead of tracking a
'countries_string') but first step would be some comments explaining
what this is trying to achieve.

Cheers! -Damian