[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:3 atagar]:
 > > +        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])

 Done.

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

 I did consider this, wasn't sure if I should add another exception class.
 Done.

 >
 > > +      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 pop_* methods treat the space as a seperator, GETCONF responses can be
 of the form

 {{{
 250 DataDirectory=/tmp/fake dir
 }}}

 The pop_mapping would return ("DataDirectory", "/tmp/fake") and leave
 "dir" in self._remainder for the next pop. Until some other control
 message's response has similar formatting, I'd like to leave this as it
 is.

 > 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)?
 Multiple values would've failed. I missed that. Implemented it now.
 I'm not sure what a quoted value would be. Can't find any, yet.

 > > +      :class:`stem.socket.InvalidRequest` if configuration option
 requested was invalid.
 >
 > Extra punctuation (the :param: and :returns: haven't been including it
 so far)
 Fixed.

 > The torrc_path is unused.

 ugh, removed.

 > Does this test work when you run with a ControlSocket target?
 It wouldn't have, fixed it to make it work.

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