[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:
Replying to [comment:7 atagar]:
> Hi Ravi. Looks close! Mostly just minor issues, though the API change
(and supporting the accursed HiddenService options) will require some more
work.
>
> ... since None will be a commonly desired default value. The following
except clause should be change too, of course.
Ah, yes. Fixed.
> 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.
Done.
I added optional keyword arguments to the stem.response.convert method
which get passed on to the Response classes' _parse_message method.
> Good, though we should add stem.socket.InvalidArguments to get_info()'s
pydocs too.
Done.
> > + 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.
Ah. I've removed this check entirely. Were you suggesting something else?
> > +def _split_line(line):
> ...
> 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.
Fine.
> Are you sure that Tor provides multiple 552 lines for multiple invalid
keys rather than a single 552 that lists all of them?
Yes.
> Please include a test case in both the unit and integ tests for a single
request with multiple unrecognized keys.
Done. Found a bug while doing this. Fixed it.
> We should also include integ tests for a LineList attribute (like
ExitPolicy),
I added two fake NodeFamily options to the BASE_TORRC to do this.
> and if we can test for the HiddenService attributes then that would be
useful since they're so weird.
I'm thinking it would be better to this if/when we have a hidden service
target?
If you want to add this to BASE_TORRC, I could do that (quickly).
> *If* you want to switch to just 'str => set' mappings (to keep the value
deduplication) then the following should do the trick...
Maybe doing deduplication isn't such a good idea. I initially decided to
do it because
{{{
getconf log log
250-Log=notice stdout
250 Log=notice stdout
}}}
But, if something like exitpolicy has duplicate lines in the configuration
file like
{{{
ControlPort 9100
ExitPolicy accept 34.3.4.5
ExitPolicy accept 3.4.53.3
ExitPolicy accept 3.4.53.3
ExitPolicy reject 23.245.54.3
}}}
then the deduplication will remove the duplicate in the configuration file
{{{
getconf exitpolicy exitpolicy
250-ExitPolicy=accept 34.3.4.5
250-ExitPolicy=accept 3.4.53.3
250-ExitPolicy=accept 3.4.53.3
250-ExitPolicy=reject 23.245.54.3
250-ExitPolicy=accept 34.3.4.5
250-ExitPolicy=accept 3.4.53.3
250-ExitPolicy=accept 3.4.53.3
250 ExitPolicy=reject 23.245.54.3
}}}
Not 100% sure if this is the right thing to do, but, I would assume if
there's ever a tool that checks the configuration file for duplicates etc
(a torrclint), it would want to get the duplicates as is.
> Very, very minor thing but please start :param: and :var: entries with a
lowercase (that's what I've been doing throughout the codebase).
Whoops, sorry. I'm trying. (When good habits turn bad...)
>
> > +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?
>
There are other things that could make a request invalid, such as this in
SETCONF responses.
https://gitweb.torproject.org/torspec.git/blob/master:/control-
spec.txt#l223
{{{
223 "513 syntax error in configuration values" reply on syntax error,
or a
224 "553 impossible configuration setting" reply on a semantic error.
}}}
> 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...
Done. I added that because of the possibility of getting multiple status
codes. In retrospect, I don't think that ever happens either.
> 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'?
Okay. Done.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/6114#comment:8>
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