[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