[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:

 Looks great! Only a few minor quibbles or questions.

 > - I added another exception class for when the request made by the user
 is invalid instead of raising a ProtocolError. (Since, this isn't really
 an error with the tor following protocol). If this the right way to do it,
 the GETINFO code should probably use it too.

 Good idea. Agreed that we should add it to GETINFO.

 > - That said, atagar, would it be easier for you if I did all my
 controller class work in a single branch? Since, merging multiple branches
 dealing with the same code might be painful? Or is that not a problem?

 Lets do separate branches. They'll cause minor merge conflicts, but they
 should be easy for me to resolve since we have tests to exercise all of
 this.

 > +  """
 > +  Base Exception class for invalid requests
 > +  """
 > +  pass

 Turns out that you actually don't need the 'pass'. If I still have other
 exceptions with it then please correct them.

 > +        if code == '552':
 > +          try:
 > +            # to parse: 552 Unrecognized configuration key "zinc"
 > +            unrecognized_keywords.append(re.search('"([^"]+)"',
 line).groups()[0])

 It looks like the error message is static, and if it changes then we'll
 likely break, so lets instead change this to...

 if code == '552' and line.startswith('552 Unrecognized configuration key
 "') and line.endswith('"'):
   unrecognized_keywords.append(line[36:-1])

 If you think that this is more hacky then I'm happy to discuss it. :)

 > +        raise stem.response.InvalidRequest("GETCONF request contained
 unrecognized keywords: %s\n" \
 > +            % ', '.join(unrecognized_keywords))

 Hm, I wonder if the caller would find the unrecognized_keywords to be
 useful as an attribute. Thoughts? If so then maybe we should have an
 InvalidArgument or InvalidInput as a InvalidRequest subclass. On the other
 hand, maybe a bad idea. Up to you.

 > +      if '=' in line:
 > +        if line[line.find("=") + 1] == "\"":
 > +          key, value = line.pop_mapping(True)
 > +        else:
 > +          key, value = line.split("=", 1)
 > +      else:
 > +        key, value = (line, None)

 Alternatively I'm pretty sure that this could be...

 if line.is_next_mapping(quoted = True):
   key, value = line.pop_mapping(True)
 elif line.is_next_mapping(quoted = False):
   key, value = line.pop_mapping(False)
 else:
   key, value = line.pop(), None

 We could change pop_mapping to allow for "maybe its a quoted value or
 maybe not" if that would help.

 The unit tests that you have look great, but should include one for quoted
 values (what is an example of a configuration option that's quoted?).
 Also, what about multiple getconf values (for instance, ExitPolity)?

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

 Extra punctuation (the :param: and :returns: haven't been including it so
 far)

 > +      control_port = str(runner.get_tor_socket().get_port())
 > +      torrc_path = runner.get_torrc_path()
 > +      self.assertEqual(control_port,
 controller.get_conf("ControlPort"))
 > +      self.assertEqual(control_port, controller.get_conf("ControlPort",
 "la-di-dah"))

 The torrc_path is unused. Does this test work when you run with a
 ControlSocket target?

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