[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #3280 [Torperf]: Make the new Python Torperf write torrc's



#3280: Make the new Python Torperf write torrc's
-------------------------+--------------------------------------------------
 Reporter:  karsten      |          Owner:  tomb        
     Type:  enhancement  |         Status:  needs_review
 Priority:  normal       |      Milestone:              
Component:  Torperf      |        Version:              
 Keywords:               |         Parent:  #2565       
   Points:               |   Actualpoints:              
-------------------------+--------------------------------------------------

Comment(by atagar):

 Hi Tom. Looks like a nice start. I checked your series of commits (via
 'git diff 079de66 7159cb1') and here's my two cents...

 ========================================
 README
 ========================================

 'controling' and 'necisary' are misspellings in the header

 > BUGS:  bugs that I know about that could impact normal use, please add
 > any major bugs you spot to this file.

 It would be better if they filed trac tickets for any issues. Especially
 since they're not likely to have commit access. ;)

 ========================================
 torperf.py
 ========================================

 > # as much code as possible is stolen from the arm project

 Heh, cute. ;)

 I'll be moving arm's utilities to Stem so in a few months you should have
 a proper library to import from rather than duplicating modules.

 > # Global Vars are in ALL CAPS

 This is a very, very common convention for all languages. No need to
 comment it.

 > DEF_WD = os.path.expanduser("~/torperf/wd/")

 What is the 'DEF_' prefix for? It seems to be fairly common and my first
 guess is 'defined', but that wouldn't make sense. A comment explaining the
 purpose of those globals would be nice.

 > DEF_LOG_DUMP_FQ_FILENAME = DEF_WD + "log"

 Very minor thing but 'log' would be a filename and
 '/home/user/torperf/wd/log' is a path. I'd call this something like
 'LOG_PATH' instead.

 > """
 > Dumps the current torrc configuration into the log
 > We recomment that you dump conf and keep it with your data so that
 > you can replicate experiments.
 > """

 Very minor thing but there's some trailing whitespace after the quotes.
 There's a bit of a holy war about extra whitespace and my feeling is that
 lines should have the same indentation level as the code around it.
 However, you pick neither of the warring whitespace camps and seem to
 include/drop whitespace randomly. ;)

 > torperfConfigKeys = list(config.getKeys())
 > torperfConfigKeys.sort()

 You could also do...
 torperfConfigKeys = sorted(config.getKeys())

 > def _dumpConfig():

 I'd probably replace this entire function with...

 config = util.conf.getConfig("torperf")
 dumpLines = ["Torperf Configuration:"]

 configKeys = sorted(config.getKeys())

 if configKeys:
   dumpLines += ["%s -> %s" % (key, config.contents[key]) for key in
 configKeys]
 else:
   dumpLines[0] += " None"

 util.log.log(util.log.DEBUG, "\n".join(dumpLines))

 > # --------------------------- end boot stuff -----------------------

 I don't know what this comment means.

 > # TODO - tomb: shouldn't config.update() do this?
 > confKeys = config.getKeys()
 > for key in confKeys:
 >   if not key in CONFIG:
 >     CONFIG[key] = config.getValue(key)

 No, config.update() won't and there seems to be a misunderstanding here of
 how the config works.

 Every file that uses configuration options has its own dictionary (CONFIG
 in this case) which tells us two things: which configuration values this
 file cares about and what the default values are. Like all constant values
 this should never be edited.

 What you're doing here is adding anything that's in configuration file
 that we loaded but not in CONFIG to the CONFIG dictionary. To use this
 properly you should...

 - Remove the above code snippet.
 - Add "run.ids" to the CONFIG dictionary at the top of the file since you
 use it a little later.
 - Don't pass CONFIG into 'util.run.Run'. No other file should care about
 the default CONFIG values that torperf.py uses internally.

 > runIds = config.getValue("run.ids", default = "")
 > runIds = runIds.split(",")

 Since you have the default value this could be combined into...
 runIds = config.getValue("run.ids", default = "").split(",")

 or better you could do...
 runIds = config.getIntCSV("run.ids", [])

 if you want integer runIds or 'getStrCSV' instead if you want to allow
 arbitrary strings.

 > util.log.log(util.log.DEBUG, "Run IDs: " + CONFIG["run.ids"])

 This will currently crash if "run.ids" isn't in your config file. This is
 probably fine since you check it above (by exiting with "at least one run
 id must be specified in configuration"), but this is kinda funky.

 > runs = []

 Not used, you can delete this. By the way, running pylint with your code
 will catch a lot of issues. Here's how I run it...
 pylint --indent-string="  " --disable=C,R myScript.py | less

 > runId = runId.lstrip()

 This is alright, but I'd probably sanitize the values earlier. Also, are
 you fine with empty ids? For instance, a config entry like...
 run.ids 1,2,,3

 > run = util.run.Run(runId, CONFIG, socksPort, controlPort)

 Besides the CONFIG, passing in the socksPort and controlPort shouldn't be
 necessary since it's just the "general.startSocksPorts" and
 "general.startControlPorts" config values. By the way, aren't separate
 torperf instances supposed to have unique ports? Otherwise running them
 all at the same time will fail (tor will fail to bind to the port and
 refuse to start).

 ========================================
 util/run.py
 ========================================

 > config = conf.getConfig("torperf")

 This is never used in the file

 > self.CONFIG = CONFIG # DO NOT MODIFY CONFIG, it might explode

 Right, because that would be concurrent edits of the 'torperf.py' default
 config values which were synced to 'config' in an addhoc manner. ;)

 Use the 'config' global that you made earlier. It has the same
 configuration values and has locking to safely handle concurrent
 modifications.

 >   def getConfig(self, key):
 >       runPrefix = "run." + self.runId + "."
 >       if runPrefix + key in self.CONFIG:
 >           return self.CONFIG[runPrefix + key]
 >       else:
 >           if "general." + key in self.CONFIG:
 >               return self.CONFIG["general." + key]
 >           else:
 >               return None


 If you used the config file instead (ie, the 'config' rather than 'CONFIG'
 variable) then this would all be a little nicer.

 def getConfig(self, key):
   prefix = "run.%s." % self.runId
   default = config.get("general.%s" % key)
   return config.get(prefix + key, default)

 > def writeTorrc(self):

 It's probably overkill but another option is...
 https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torConfig.py#l910

 If you're gonna stick with this then I'd suggest instead going with...
 torrcLines = ["DataDirectory %s" % self.dataDirectory,
               "SocksPort %s" % str(self.socksPort),
               ... etc... ]

 fh.write("\n".join(torrcLines))

 > os.chdir(self.dataDirectory)

 This doesn't look necessary until, maybe, the download later (I'm not
 really clear what most of this is doing - commenting would be a good
 idea). Also, like outputFh and dumpFh this looks like it'll be broken with
 concurrent runs.

 ========================================
 util/test.py
 ========================================

 This is an empty python script. Why is it here?

 I blame trac for any whitespace/line wrapping funkyness in the above.
 Cheers! -Damian

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/3280#comment:3>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs