[tor-dev] Stem code review 2013-01-03

[Sean Robinson]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20130105/09db9aea/attachment.html>