[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_revision
Priority: normal | Milestone:
Component: Stem | Version:
Keywords: | Parent:
Points: | Actualpoints:
--------------------+-------------------------------------------------------
Changes (by atagar):
* status: needs_review => needs_revision
Comment:
Hi Ravi. Looks close! Mostly just minor issues, though the API change (and
supporting the accursed HiddenService options) will require some more
work.
> + def get_conf(self, param, default = None):
This should be...
{{{
def get_conf(self, param, default = UNDEFINED):
}}}
... since None will be a commonly desired default value. The following
except clause should be change too, of course.
> + :returns:
> + Response depends upon how we were called as follows...
> +
> + * str with the response if our param was a str
> + * dict with the param => response mapping if our param was a list
> + * default if one was provided and our call failed
Is this right? If I query a LineList entry like the 'ExitPolicy' then do I
get a str or a list? The dict response is certainly more complicated since
GetConfResponse provides str => (str or list) mappings, which will be
problematic for users since it means that they need to check the type
before it can be used for anything.
Please see arm's src/util/torTools.py, the getOption() and getOptionMap()
methods (and feel free to steal any code or pydocs you want!). I suspect
that will provide a better API for users.
> + :raises:
> + :class:`stem.socket.ControllerError` if the call fails, and we
weren't provided a default res
> + :class:`stem.socket.InvalidArguments` if configuration options
requested was invalid
Good, though we should add stem.socket.InvalidArguments to get_info()'s
pydocs too.
> + if requested_params != reply_params:
> + requested_label = ", ".join(requested_params)
> + reply_label = ", ".join(reply_params)
> +
> + raise stem.socket.ProtocolError("GETCONF reply doesn't match
the parameters that we requested. Queried '%s' but got '%s'." %
(requested_label, reply_label))
This actually doesn't always apply to GETINFO responses. Read arm's
getOptionMap() pydocs and you'll understand. The HiddenService options are
a real pain in the ass.
> +def _split_line(line):
> + if line.is_next_mapping(quoted = False):
> + return line.split("=", 1) # TODO: make this part of the
ControlLine?
> + elif line.is_next_mapping(quoted = True):
> + return line.pop_mapping(True).items()[0]
> + else:
> + return (line.pop(), None)
Lets do this in _parse_message(). Helper methods are nice, but in this
case neither the helper nor _parse_message() are especially complicated,
so it mostly just gives another place look to figure out how GETCONF is
parsed.
As discussed on irc, we should cite the ticket you filed about quotes
here.
> + if not self.is_ok():
> + unrecognized_keywords = []
> + for code, _, line in self.content():
> + if code == "552" and line.startswith("Unrecognized
configuration key \"") and line.endswith("\""):
> + unrecognized_keywords.append(line[32:-1])
> +
> + if unrecognized_keywords:
> + raise stem.socket.InvalidArguments("552", "GETCONF request
contained unrecognized keywords: %s\n" \
> + % ', '.join(unrecognized_keywords), unrecognized_keywords)
> + else:
> + raise stem.socket.ProtocolError("GETCONF response contained a
non-OK status code:\n%s" % self)
Are you sure that Tor provides multiple 552 lines for multiple invalid
keys rather than a single 552 that lists all of them? Please include a
test case in both the unit and integ tests for a single request with
multiple unrecognized keys. We should also include integ tests for a
LineList attribute (like ExitPolicy), and if we can test for the
HiddenService attributes then that would be useful since they're so weird.
> + while remaining_lines:
> + line = remaining_lines.pop(0)
> +
> + key, value = _split_line(line)
> + entry = self.entries.get(key, None)
> +
> + if type(entry) == str and entry != value:
> + self.entries[key] = [entry]
> + self.entries[key].append(value)
> + elif type(entry) == list and not value in entry:
> + self.entries[key].append(value)
> + else:
> + self.entries[key] = value
I see the appeal of having both a 'str => str' and 'str => list' mapping,
but I suspect that it'll actually make things harder on the caller. *If*
you want to switch to just 'str => set' mappings (to keep the value
deduplication) then the following should do the trick...
{{{
key, value = _split_line(line)
self.entries.setdefault(key, set()).add(value)
}}}
'setdefault' gets the value of 'key' if it exists and, if it doesn't,
inserts the second value into the dict and returns that. Something new
that I learned at the python interest group last night - neat trick that
I'll probably abuse immensely. :P
> + |- InvalidRequest - Invalid request.
> + +- InvalidArguments - Invalid request parameters.
> +- SocketError - Communication with the socket failed.
Missing pipe...
{{{
+ |- InvalidRequest - Invalid request.
+ | +- InvalidArguments - Invalid request parameters.
+- SocketError - Communication with the socket failed.
}}}
> + :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
Very, very minor thing but please start :param: and :var: entries with a
lowercase (that's what I've been doing throughout the codebase).
> +class InvalidRequest(ControllerError):
> +class InvalidArguments(InvalidRequest):
Now that I think about it more, what could make a request invalid besides
its arguments? Maybe this division isn't so useful... thoughts?
> + :param str code: The error code returned by Tor (if applicable)
> + :param str message: The error message returned by Tor (if
applicable) or a
> + human readable error message
Instead of saying '(if applicable)' it's usually better to tell the user
what exactly will happen. For instance...
{{{
:param str code: error code provided by tor, this is None if nothing was
provided
}}}
However, is there any case where we won't have a status code and message?
I can't think of any, so lets simplify it to...
{{{
:param str code: error code provided by tor
:param str message: message describing the issue
}}}
> + socket = runner.get_tor_socket()
> + if isinstance(socket, stem.socket.ControlPort):
> + socket = str(socket.get_port())
> + config_key = "ControlPort"
Dynamic languages like python allow you to reuse a variable for completely
different things, but it hurts code readability. Maybe call the first
'socket' and the second 'connection_value'?
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6114#comment:7>
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