[tor-bugs] #6114 [Stem]: Implement GETCONF parsing in Stem

[Tor Bug Tracker & Wiki]

#6114: Implement GETCONF parsing in Stem
--------------------+-------------------------------------------------------
 Reporter:  neena   |          Owner:  neena       
     Type:  task    |         Status:  needs_review
 Priority:  normal  |      Milestone:              
Component:  Stem    |        Version:              
 Keywords:          |         Parent:              
   Points:          |   Actualpoints:              
--------------------+-------------------------------------------------------
Changes (by neena):

  * status:  needs_revision => needs_review


Comment:

 Replying to [comment:7 atagar]:
 > Hi Ravi. Looks close! Mostly just minor issues, though the API change
 (and supporting the accursed HiddenService options) will require some more
 work.
 >
 > ... since None will be a commonly desired default value. The following
 except clause should be change too, of course.
 Ah, yes. Fixed.

 > Please see arm's src/util/torTools.py, the getOption() and
 getOptionMap() methods (and feel free to steal any code or pydocs you
 want!). I suspect that will provide a better API for users.
 Done.
 I added optional keyword arguments to the stem.response.convert method
 which get passed on to the Response classes' _parse_message method.

 > Good, though we should add stem.socket.InvalidArguments to get_info()'s
 pydocs too.
 Done.

 > > +        raise stem.socket.ProtocolError("GETCONF reply doesn't match
 the parameters that we requested. Queried '%s' but got '%s'." %
 (requested_label, reply_label))
 >
 > This actually doesn't always apply to GETINFO responses. Read arm's
 getOptionMap() pydocs and you'll understand. The HiddenService options are
 a real pain in the ass.
 Ah. I've removed this check entirely. Were you suggesting something else?

 > > +def _split_line(line):
 > ...
 > Lets do this in _parse_message(). Helper methods are nice, but in this
 case neither the helper nor _parse_message() are especially complicated,
 so it mostly just gives another place look to figure out how GETCONF is
 parsed.
 Fine.

 > Are you sure that Tor provides multiple 552 lines for multiple invalid
 keys rather than a single 552 that lists all of them?
 Yes.
 > Please include a test case in both the unit and integ tests for a single
 request with multiple unrecognized keys.
 Done. Found a bug while doing this. Fixed it.
 > We should also include integ tests for a LineList attribute (like
 ExitPolicy),
 I added two fake NodeFamily options to the BASE_TORRC to do this.
 > and if we can test for the HiddenService attributes then that would be
 useful since they're so weird.
 I'm thinking it would be better to this if/when we have a hidden service
 target?
 If you want to add this to BASE_TORRC, I could do that (quickly).

 > *If* you want to switch to just 'str => set' mappings (to keep the value
 deduplication) then the following should do the trick...

 Maybe doing deduplication isn't such a good idea. I initially decided to
 do it because

 {{{
 getconf log log
 250-Log=notice stdout
 250 Log=notice stdout
 }}}
 But, if something like exitpolicy has duplicate lines in the configuration
 file like
 {{{
 ControlPort 9100
 ExitPolicy accept 34.3.4.5
 ExitPolicy accept 3.4.53.3
 ExitPolicy accept 3.4.53.3
 ExitPolicy reject 23.245.54.3
 }}}
 then the deduplication will remove the duplicate in the configuration file
 {{{
 getconf exitpolicy exitpolicy
 250-ExitPolicy=accept 34.3.4.5
 250-ExitPolicy=accept 3.4.53.3
 250-ExitPolicy=accept 3.4.53.3
 250-ExitPolicy=reject 23.245.54.3
 250-ExitPolicy=accept 34.3.4.5
 250-ExitPolicy=accept 3.4.53.3
 250-ExitPolicy=accept 3.4.53.3
 250 ExitPolicy=reject 23.245.54.3
 }}}

 Not 100% sure if this is the right thing to do, but, I would assume if
 there's ever a tool that checks the configuration file for duplicates etc
 (a torrclint), it would want to get the duplicates as is.

 > Very, very minor thing but please start :param: and :var: entries with a
 lowercase (that's what I've been doing throughout the codebase).
 Whoops, sorry. I'm trying. (When good habits turn bad...)

 >
 > > +class InvalidRequest(ControllerError):
 > > +class InvalidArguments(InvalidRequest):
 >
 > Now that I think about it more, what could make a request invalid
 besides its arguments? Maybe this division isn't so useful... thoughts?
 >
 There are other things that could make a request invalid, such as this in
 SETCONF responses.
 https://gitweb.torproject.org/torspec.git/blob/master:/control-
 spec.txt#l223

 {{{
  223   "513 syntax error in configuration values" reply on syntax error,
 or a
  224   "553 impossible configuration setting" reply on a semantic error.
 }}}

 > However, is there any case where we won't have a status code and
 message? I can't think of any, so lets simplify it to...
 Done. I added that because of the possibility of getting multiple status
 codes. In retrospect, I don't think that ever happens either.

 > Dynamic languages like python allow you to reuse a variable for
 completely different things, but it hurts code readability. Maybe call the
 first 'socket' and the second 'connection_value'?
 Okay. Done.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6114#comment:8>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online