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

Re: [tor-bugs] #5454 [Stem]: ExitPolicy Class for stem

#5454: ExitPolicy Class for stem
-------------------------+--------------------------------------------------
 Reporter:  gsathya      |          Owner:  atagar        
     Type:  enhancement  |         Status:  needs_revision
 Priority:  normal       |      Milestone:                
Component:  Stem         |        Version:                
 Keywords:               |         Parent:                
   Points:               |   Actualpoints:                
-------------------------+--------------------------------------------------
Changes (by atagar):

  * status:  needs_review => needs_revision


Comment:

 Hi gsathya, sorry about the delay.

 I would suggest rebasing your branch onto the current master and squash
 what you have so far...
 git rebase -i master

 Generally a commit corresponds to a single task, so squashing all of your
 commits is appropriate (all of your work so far have been formatting fixes
 for the arm module we're copying).

 Keeping a clean history isn't vital (I can squash or change commits myself
 prior to pushing), but it makes our repository easier to make sense of.
 Just in case you're unaware, you can easily fix your last commit with...
 git commit --amend

 (this takes anything that is staged and applies it to your last commit,
 making it handy for correcting your prior work)

 > exit_policies = stem.version.ExitPolicy()

 Copy and paste error. ;)

 Header looks good for now. It might be good to tweak it later but that is
 usually the last thing I do since function names and usage are likely to
 change a few times.

 > elif policy.is_ip_wildcard and self.is_port_wildcard: return False

 This is just an attribute of the ExitPolicyLine so the
 "self.is_port_wildcard" will cause an error.

 > def isExitingAllowed(self):

 Should follow the underscore convention.

 On a quick glance the formatting looks good so we can move on to the
 testing. Only unit tests are needed (integ tests don't make sense for
 this), but we need a lot of good ones since exit policies are highly error
 prone. This is actually the vast majority of the work behind this task.

 > I'm not sure of the difference - I'll read up on the microdescriptor
 exit policies, but it looks like we can subclass ExitPolicyLine for the
 microdescriptor exit policies?

 From what I understand the microdescriptor policy is simply a series of
 accepted and rejected port numbers or ranges (no ip addresses nor keywords
 like private) though you should read the spec to figure out exactly what
 it consists of.

 My guess is that we'll want an abstract parent class (ExitPolicyLine) and
 a couple for the implementations (ConsensusExitPolicyLine which is what
 ExitPolicyLine is now and MicrodescriptorExitPolicyLine for
 microdescriptors). But maybe I'm wrong. Please read through the spec
 though and see if something else makes better sense (and better names!).

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5454#comment:4>
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