[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
[tor-dev] Stem code review 2013-01-03
Stem devs,
This is a review of commits made to Stem from 2012-12-22 through 2013-01-03. Happy new year.
I'm a fan of the new Controller.get_circuits() method. It's a nice and clever re-use of internal code. And it makes circuit-oriented method tests much more readable. Controller.get_circuit() is another good idea. A positive feedback loop...
Using defaults for getters is betters[0]. This allows developers to choose whether to accept the exception or use their own reasonable default.
I like the synchronous[1] interface to Controller.extend_circuit(). It's a good idea and a good internal re-use of event handling. Even better, the sync interface is optional. I did find the 'assert extended == "EXTENDED"' test after is_ok() to be strange, I cannot find where Tor has ever returned anything other than "250 EXTENDED" on success for an EXTENDCIRCUIT command. This may be more verification of results than is warranted. If Damian and Ravi agree, I would be happy to submit a patch removing this line.
With the recent changes[2] to ExitPolicy and related classes, I dove into research on this topic. But, there is still much I don't understand, so please be careful with the following comments.
1) There is a discrepancy in class naming (i.e. MicroExitPolicyRule vs MicrodescriptorExitPolicy) about which I vote to rename MicrodescriptorExitPolicy to MicroExitPolicy.
2) How can a lightweight class be a subclass of a heavy class and be lightweight? MicrodescriptorExitPolicy is a subclass of ExitPolicy and MicroExitPolicyRule is a subclass of ExitPolicyRule. I concentrated on MicroExitPolicyRule and ExitPolicyRule.
What I found:
a) There are a few small attributes (i.e. _address_type, _mask_bin, _addr_bin, _mask, _masked_bits) in ExitPolicyRule and not in MicroExitPolicyRule.
b) There is minimal extra processing of get_*() methods in ExitPolicyRule.
c) Private methods (i.e. _get_address_bin, _apply_portspec, _apply_addrspec, _get_mask_bin) from ExitPolicyRule are still in MicroExitPolicyRule and
cause exceptions (e.g. "AttributeError: 'MicroExitPolicyRule' object
has no attribute '_mask_bin'") when called.
d) However,
i) sys.getsizeof(ExitPolicy("accept 1.1.1.1:34")) == 64 bytes
ii) sys.getsizeof(MicroExitPolicyRule(True, 34, 34)) == 64 bytes
iii) I made a mutant MicroExitPolicyRule that was a direct subclass of object and sys.getsizeof(MicroExitPolicyRule(True, 34, 34)) == 64 bytes
I'm not sure these MicroExitPolicy* classes can be considered lightweight. Would a class with least common attributes (e.g. BaseExitPolicyRule) from which ExitPolicyRule and MicroExitPolicyRule descended make sense. But, considering the size results in 2)d), is it worth the time to optimize this area?
And lastly, I want to quote my favorite line from these recent commits[last]: address = address.lstrip('[').rstrip(']')Â Wait, you can do that?
[0]: https://gitweb.torproject.org/stem.git/commit/ffdd61ea41ca844047ac57725639a7cf0e22d41d
[1]: https://gitweb.torproject.org/stem.git/commit/f716fedc97ea3cc522571ad8bd505e847379c0d7
[2]: https://gitweb.torproject.org/stem.git/commit/da65f282699d3e8e763e1f3ba3c7612cc970178a
and https://gitweb.torproject.org/stem.git/commit/d35b9c737e320500fa6cfdfa874534cfbe6f1b3a
[last]: https://gitweb.torproject.org/stem.git/commit/683b1ba479f2aff3c277e0ff53bdc8b1f81664af
--
Sean Robinson
_______________________________________________
tor-dev mailing list
tor-dev@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev