[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_revision
 Priority:  normal  |      Milestone:                
Component:  Stem    |        Version:                
 Keywords:          |         Parent:                
   Points:          |   Actualpoints:                
--------------------+-------------------------------------------------------
Changes (by atagar):

  * status:  needs_review => needs_revision


Comment:

 Hi Ravi. Looks close! Mostly just minor issues, though the API change (and
 supporting the accursed HiddenService options) will require some more
 work.

 > +  def get_conf(self, param, default = None):

 This should be...

 {{{
   def get_conf(self, param, default = UNDEFINED):
 }}}

 ... since None will be a commonly desired default value. The following
 except clause should be change too, of course.

 > +    :returns:
 > +      Response depends upon how we were called as follows...
 > +
 > +      * str with the response if our param was a str
 > +      * dict with the param => response mapping if our param was a list
 > +      * default if one was provided and our call failed

 Is this right? If I query a LineList entry like the 'ExitPolicy' then do I
 get a str or a list? The dict response is certainly more complicated since
 GetConfResponse provides str => (str or list) mappings, which will be
 problematic for users since it means that they need to check the type
 before it can be used for anything.

 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.

 > +    :raises:
 > +      :class:`stem.socket.ControllerError` if the call fails, and we
 weren't provided a default res
 > +      :class:`stem.socket.InvalidArguments` if configuration options
 requested was invalid

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

 > +      if requested_params != reply_params:
 > +        requested_label = ", ".join(requested_params)
 > +        reply_label = ", ".join(reply_params)
 > +
 > +        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.

 > +def _split_line(line):
 > +  if line.is_next_mapping(quoted = False):
 > +    return line.split("=", 1) # TODO: make this part of the
 ControlLine?
 > +  elif line.is_next_mapping(quoted = True):
 > +    return line.pop_mapping(True).items()[0]
 > +  else:
 > +    return (line.pop(), None)

 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.

 As discussed on irc, we should cite the ticket you filed about quotes
 here.

 > +    if not self.is_ok():
 > +      unrecognized_keywords = []
 > +      for code, _, line in self.content():
 > +        if code == "552" and line.startswith("Unrecognized
 configuration key \"") and line.endswith("\""):
 > +          unrecognized_keywords.append(line[32:-1])
 > +
 > +      if unrecognized_keywords:
 > +        raise stem.socket.InvalidArguments("552", "GETCONF request
 contained unrecognized keywords: %s\n" \
 > +            % ', '.join(unrecognized_keywords), unrecognized_keywords)
 > +      else:
 > +        raise stem.socket.ProtocolError("GETCONF response contained a
 non-OK status code:\n%s" % self)

 Are you sure that Tor provides multiple 552 lines for multiple invalid
 keys rather than a single 552 that lists all of them? Please include a
 test case in both the unit and integ tests for a single request with
 multiple unrecognized keys. We should also include integ tests for a
 LineList attribute (like ExitPolicy), and if we can test for the
 HiddenService attributes then that would be useful since they're so weird.

 > +    while remaining_lines:
 > +      line = remaining_lines.pop(0)
 > +
 > +      key, value = _split_line(line)
 > +      entry = self.entries.get(key, None)
 > +
 > +      if type(entry) == str and entry != value:
 > +        self.entries[key] = [entry]
 > +        self.entries[key].append(value)
 > +      elif type(entry) == list and not value in entry:
 > +        self.entries[key].append(value)
 > +      else:
 > +        self.entries[key] = value

 I see the appeal of having both a 'str => str' and 'str => list' mapping,
 but I suspect that it'll actually make things harder on the caller. *If*
 you want to switch to just 'str => set' mappings (to keep the value
 deduplication) then the following should do the trick...

 {{{
   key, value = _split_line(line)
   self.entries.setdefault(key, set()).add(value)
 }}}

 'setdefault' gets the value of 'key' if it exists and, if it doesn't,
 inserts the second value into the dict and returns that. Something new
 that I learned at the python interest group last night - neat trick that
 I'll probably abuse immensely. :P

 > +    |- InvalidRequest - Invalid request.
 > +       +- InvalidArguments - Invalid request parameters.
 >      +- SocketError - Communication with the socket failed.

 Missing pipe...

 {{{
   +    |- InvalidRequest - Invalid request.
   +    |  +- InvalidArguments - Invalid request parameters.
        +- SocketError - Communication with the socket failed.
 }}}

 > +  :var str code: The error code returned by Tor (if applicable)
 > +  :var str message: The error message returned by Tor (if applicable)
 or a human
 > +    readable error message

 Very, very minor thing but please start :param: and :var: entries with a
 lowercase (that's what I've been doing throughout the codebase).

 > +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?

 > +    :param str code: The error code returned by Tor (if applicable)
 > +    :param str message: The error message returned by Tor (if
 applicable) or a
 > +      human readable error message

 Instead of saying '(if applicable)' it's usually better to tell the user
 what exactly will happen. For instance...

 {{{
   :param str code: error code provided by tor, this is None if nothing was
 provided
 }}}

 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...

 {{{
   :param str code: error code provided by tor
   :param str message: message describing the issue
 }}}

 > +      socket = runner.get_tor_socket()
 > +      if isinstance(socket, stem.socket.ControlPort):
 > +        socket = str(socket.get_port())
 > +        config_key = "ControlPort"

 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'?

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