[tor-bugs] #6114 [Stem]: Implement GETCONF parsing in Stem
Tor Bug Tracker & Wiki
torproject-admin at torproject.org
Thu Jun 28 16:27:30 UTC 2012
#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:
So many changes to review. Oh well. Once more unto the breach, dear
friends, once more.
Just for clarification, I'm reviewing 'git diff origin/master neena
/getconf-parsing'.
>> 2. get_conf_map with parameters...
>> - param (str or list for batch call)
>> - default (object)
>> }}}
>
> Not sure why you didn't add a 'multiple' there. Did you want it to
default
> to multiple, or mixed (string or list of strings)?
> (I added a multiple parameter)
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.
>> On reflection, the check is appropriate for the first of those two
methods.
>
> Added, to both of the methods. It checks as long as none of the
requested
> values were weird, i.e., needed to be retrieved by querying for a
> different value.
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...
{{{
not bool(len(set(self._mapped_config_keys.values()))) =>
not bool(len(["HiddenServiceOptions"])) =>
not bool(1) =>
not True
}}}
> + _mapped_config_keys = {
> + "HiddenServiceDir": "HiddenServiceOptions",
> + "HiddenServicePort": "HiddenServiceOptions",
> + "HiddenServiceVersion": "HiddenServiceOptions",
> + "HiddenServiceAuthorizeClient": "HiddenServiceOptions",
> + "HiddenServiceOptions": "HiddenServiceOptions"
> + }
There's no reason for this to be an instance variable. Lets make it a
global constant.
> + :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()).
> + 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]
Three things...
a. You're checking if 'param.lower()' is among the keys but fetching just
'param', you you'll get a KeyError if the case doesn't match. We should
probably just do 'param = param.lower() # case insensitive' right away.
b. The default check for 'in' is the keys...
{{{
>>> my_dict = {"hello": "world"}
>>> "hello" in my_dict
True
>>> "world" in my_dict
False
}}}
c. An alternative is...
{{{
param = param.lower() # case insensitive
return self.get_conf_map(self._mapped_config_keys.get(param, param),
default, multiple)[param]
}}}
Though your version, with an explicit 'if/else', might be a bit more
readable. Up to you.
+ def get_conf_map(self, param, default = UNDEFINED, multiple = False):
Did you find arm's description of the various config option types to be
helpful? If so then lets preserve it here (modified slightly to reflect
what we're doing here, further improvements welcome)...
{{{
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.
}}}
> + :returns:
> + Response depends upon how we were called as follows...
> +
> + * dict with param (str) => response mappings (str) if multiple
was False
> + * dict with param (str) => response mappings (list) if multiple
was True
> + * dict with param (str) => default mappings if a default value
was provided and our call failed
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
}}}
> + 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.
Note that if we have a default parameter then we should *never* raise an
exception (our caller won't be trying to catch one).
> + requested_params = set(map(lambda x: x.lower(), param))
> + reply_params = set(map(lambda x: x.lower(),
response.entries.keys()))
You don't need the lamdas.
{{{
>>> map(str.lower, ["HELLO", "WORLD"])
['hello', 'world']
}}}
> + 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.
> + * :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.
Btw, to make the documentation all you need to do is...
{{{
cd stem/docs
make html
}}}
Then point firefox at it, in my case
"file:///home/atagar/Desktop/stem/docs/_build/html/index.html". If you see
something that looks out of date then do a 'make clean' first (the only
thing that I've ever seen get stale due to caching is the "[source]"
contents).
> + """
> + Reply for a GETCONF query.
> +
> + :var dict entries: mapping between the queried options (string)
and...
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...
}}}
> + 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 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.
> +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.
> + with runner.get_tor_controller() as controller:
> + # successful single query
> + socket = runner.get_tor_socket()
Nope, definite badness here. Two issues...
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.
Maybe we want a '_get_test_case(controller)' method which returns the
(config_key, expected_value) tuple? Might make things a bit more readable
- up to you.
In the future I'll probably break all of these tests up into individual
use cases (ie, test_getconf_batch, test_getconf_empty, etc) but guess we
should keep it similar to what I did with getinfo for now.
> +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.
> + 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.
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.
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.
Cheers! -Damian
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6114#comment:12>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tor-bugs
mailing list