[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #6114 [Stem]: Implement GETCONF parsing in Stem
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs