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

Tor Bug Tracker & Wiki torproject-admin at torproject.org
Fri Jun 29 17:48:27 UTC 2012


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

 My commit revising the code here -->
 http://repo.or.cz/w/stem/neena.git/commit/5bf45dd3cddbd02b990b3b8af4d8225f0a33d4fc

 Replying to [comment:12 atagar]:
 > The get_conf() method should have a multiple parameter, but I left it
 out of get_conf_map() because it seemed simpler for it to always provide a
 dict of 'key => [values...]'. I can see how it might be useful for batch
 calls so I'm on the fencepost about including it. Up to you.
 Making multiple = True the default for get_conf_map, but, including it as
 a parameter.

 > Good idea, though wouldn't it be better to check that an individual
 parameter isn't a key or value in self._mapped_config_keys? I'm a little
 unsure what your check is trying to do...
 >
 > > +      if not set(self._mapped_config_keys.values()) &
 >
 > Won't this always evaluate to False since you're essentially
 evaulating...

 Nope, the '&' in
 > not set(self._mapped_config_keys.values()) & requested_params
 is an intersection of both the sets. So, that reads 'if the intersection
 between requested_params and the set of keys that are mapped to other keys
 is empty'.

 > > +  _mapped_config_keys = {
 > > +      "HiddenServiceDir": "HiddenServiceOptions",
 > There's no reason for this to be an instance variable. Lets make it a
 global constant.
 It's a class variable when defined like that. But, if you feel it should
 be a global constant, I could change that.

 > > +      :class:`stem.socket.ControllerError` if the call fails, and we
 weren't provided a default response
 > Very, very minor nitpick but we don't need a comma there (both in
 get_conf() and get_conf_map()).
 Fixed.

 > > +    if param.lower() in self._mapped_config_keys.keys():
 > > +      return self.get_conf_map(self._mapped_config_keys[param],
 default, multiple)[param]
 > > +    else:
 > > +      return self.get_conf_map(param, default, multiple)[param]
 > c. An alternative is...
 > {{{
 > param = param.lower() # case insensitive
 > return self.get_conf_map(self._mapped_config_keys.get(param, param),
 default, multiple)[param]
 > }}}
 Much better. Using this.

 > {{{
 >      There's three use cases for GETCONF:
 >      - a single value is provided
 >      - multiple values are provided for the option queried
 >      - a set of options that weren't necessarily requested are returned
 (for
 >        instance querying HiddenServiceOptions gives HiddenServiceDir,
 >        HiddenServicePort, etc)
 >
 >      The vast majority of the options fall into the first two
 categories, in
 >      which case calling get_conf() is sufficient. However, for batch
 queries or
 >      the special options that give a set of values this provides back
 the full
 >      response. As of tor version 0.2.1.25 HiddenServiceOptions was the
 only
 >      option like this.
 >
 >      The get_conf() and get_conf_map() functions both try to account for
 these
 >      special mappings, so queried like get_conf("HiddenServicePort")
 should
 >      behave as you'd expect.
 > }}}
 Stolen.

 > The 'dict with param (str) => ' is a bit redundant. Lets shorten this a
 bit to something like...
 > {{{
 > :returns:
 >   Response depends upon how we were called as follows...
 >
 >   * dict of 'config key => value' mappings, the value is a list if
 'multiple' is True and otherwise is a str of just the first value
 >   * default if one was provided and our call failed
 > }}}
 Nicked.

 > > +      if param == [""] or param == []:
 > > +        raise stem.socket.InvalidRequest("Received empty parameter")
 >
 > Hm. If 'param == []' then we should probably just return an empty
 dictionary. If 'param == [""]' then that depends, does "GETCONF " error or
 provide nothing? If the former then that should return an empty dictionary
 too.
 I was confused about how this should behave, decided to do this because
 the get_info does it too
 From def test_getinfo in test/integ/control/controller.py
 {{{
       # empty input

       self.assertRaises(stem.socket.ControllerError, controller.get_info,
 "")
       self.assertEqual("ho hum", controller.get_info("", "ho hum"))

       self.assertEqual({}, controller.get_info([]))
       self.assertEqual({}, controller.get_info([], {}))
 }}}
 Want to talk about this on IRC before we decide what to do. Accepting
 empty
 strings will be a little tricky

 > Note that if we have a default parameter then we should *never* raise an
 exception (our caller won't be trying to catch one).
 :?
 These exceptions are caught at
 {{{
      except stem.socket.ControllerError, exc:
       if default != UNDEFINED: return dict([(p, default) for p in param])
       else: raise exc
 }}}
 and then either the default is used or the exception is raised. I borrowed
 this style from the get_info method you wrote, 'tis neat.

 > > +      requested_params = set(map(lambda x: x.lower(), param))
 > You don't need the lamdas.
 > {{{
 > >>> map(str.lower, ["HELLO", "WORLD"])
 > ['hello', 'world']
 > }}}
 Ah, yes. Doing this.

 > > +        raise stem.socket.ProtocolError("GETCONF reply doesn't match
 the parameters that we requested. Queried '%s' but got '%s'." %
 (requested_label, reply_label))
 > Again, we can't raise if there's a default. How I usually handled this
 in arm was to have a 'result' and 'raised_exception' parameter. Then,
 rather than raising exceptions, I assigned it to 'raised_exception' then
 handled the "if I have a default do this, if not and I have an exception
 do that" logic at the end.
 > Personally I don't like this pattern, so if you're able to find
 something that's better then great.
 You did find something better. I used that here... see above.

 > > +    * :class:`stem.socket.InvalidArguments` the arguments given as
 input are
 > > +        invalid. Raised when converting GETINFO or GETCONF requests
 > Does this render correctly in sphinx? I've needed to sacrifice having
 sane line lengths at a few other places in the documentation because it
 then renders the first line as being part of the list, and the second line
 as a new paragraph.
 Yup, looks fine. http://ompldr.org/vZWp5Ng/tracimg-doc.png

 > Not entirely true. With the hidden service options it's the real
 parameter rather than what we queried. Another option...
 > {{{
 > Reply for a GETCONF query.
 >
 > Note that configuration parameters won't match what we queried for if
 it's one
 > of the special mapping options (ex. "HiddenServiceOptions").
 >
 > :var dict entries: mapping between the config parameter (string) and...
 > }}}
 Pirated.

 > > +      else:
 > > +        key, value = (line.pop(), None)
 > > +
 > > +      entry = self.entries.get(key, None)
 > > +
 > > +      self.entries.setdefault(key, []).append(value)
 >
 > What is the 'entry = self.entries.get(key, None)' line doing here? Looks
 like a no-op.
 I... don't remember, removed. This came in during the nasty rebase. :/

 > I forgot that we might have None values. Guess that our get_conf() and
 get_conf_map() should warn about this?
 >
 > > +  :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
 >
 > Trivial nitpicks again but should start with a lowercase (actually, I'd
 just remove the word 'The' since it reads better without it), and I'm
 pretty sure that :var: entries need to be on a single line to render
 correctly.
 My bad, sorry. This happened during the horrible rebase too.

 > > +class InvalidRequest(ControllerError):
 > > ...
 > > +  def __init__(self, code = None, message = None):
 > > +    """
 > > +    Initializes an InvalidRequest object.
 > > +
 > > +    :param str code: The error code returned by Tor
 > > +    :param str message: The error message returned by Tor
 > > +
 > > +    :returns: object of InvalidRequest class
 > > +    """
 > > +
 > > +    self.code = code
 > > +    self.message = message
 >
 > Subclasses should *always* call the __init__ for their parent class.
 Doing otherwise can lead to very, very confusing errors (speaking as
 someone who's been bitten by that a few times :P). Same goes for
 InvalidArguments rather than having it set code and message itself.
 >
 Done.
 > > +    with runner.get_tor_controller() as controller:
 > > +      # successful single query
 > > +      socket = runner.get_tor_socket()
 > 1. You're creating a control socket without closing it. This is why we
 almost always use the 'with' keyword when we get either a controller or
 socket.
 > 2. In essence what you're saying here is "Give me a controller with an
 initialized control socket" followed by "Give me an initialized control
 socket". Note that the controller already has a socket so you now have
 two. I'd suggest removing the "runner.get_tor_socket()" call and using
 "controller.get_socket()" instead.
 Fixed.

 > > +NodeFamily
 dummystemnodexyz1x1,dummystemnodexyz1x2,dummystemnodexyz1x3
 > > +NodeFamily
 dummystemnodexyz2x1,dummystemnodexyz2x2,dummystemnodexyz2x3
 >
 > Could test_get_conf() issue a SETCONF for this then test against that
 rather than making it part of our default torrc (reverting after the test
 finishes)? It feels kinda wrong to include test data here.
 yeap, okay, done.

 > > +    try:
 > > +      stem.response.convert("GETINFO", control_message)
 > > +    except stem.socket.InvalidArguments, exc:
 > > +      self.assertEqual(exc.arguments, ["blackhole"])
 > After the "stem.response.convert()" we should probably call
 "self.fail()" just in case we have a bug where it succeeds. Same goes for
 the similar getconf unit test.
 Hmm, using an else clause here.

 > Would you like us to merge your getconf-parsing, setconf-wrapper, and
 saveconf-wrapper branches all at once or one at a time? If the former then
 this'll take a while but feel free to make them all one 'tor-config'
 branch and make revisions on top of that.
 Seperately is probably better. Don't want to end with a large unmergable,
 out-of-sync branch.

 > If we are doing them individually then you'll want to make revisions to
 getconf-parsing then rebase the other two on top of that.
 Yup, that's what I'm doing.

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


More information about the tor-bugs mailing list