Opened 5 years ago

Closed 5 years ago

#6239 closed task (implemented)

Implement SETCONF/RESETCONF parsing in Stem

Reported by: neena Owned by: neena
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Stem's controller class must be able to set & reset configuration options using the SETCONF & RESETCONF commands.

I've implemented this in my git branch, here.

Child Tickets

Change History (10)

comment:1 Changed 5 years ago by neena

  • Owner changed from atagar to neena
  • Status changed from new to assigned

comment:2 Changed 5 years ago by neena

  • Status changed from assigned to needs_review

comment:3 Changed 5 years ago by neena

I've rebased my branch to the current master.
'tis ready for review.

comment:4 follow-up: Changed 5 years ago by atagar

Hi Ravi. Looks pretty good, though I think that arm had a better api for this. The SETCONF and RESETCONF do the exact same thing except for undefined values (the two controller methods would probably be combined if tor could break backward comparability). Stem's Controller shouldn't blindly mirror the control-spec if we can do better. ;)

Pushed an alternative implementation to the 'setconf-wrapper' branch of my personal repo...

https://gitweb.torproject.org/user/atagar/stem.git/shortlog/refs/heads/setconf-wrapper

Changes made on top of yours...
https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/80780e7
https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/8b3c470

Thoughts?

comment:5 follow-up: Changed 5 years ago by atagar

Oops, forgot the notes I took while reviewing it...

+def _case_insensitive_lookup(entries, key, default = UNDEFINED):

Heh, I went back and forth on adding a default parameter while using this for get_conf(), but left it out since get_conf_map() (its only other user) didn't need it. Looks like I was wrong.

Now I'm a little tempted to drop default in favor of just returning the key by default when lookup fails (since that's what all of its consumers want), but guess it's better to leave it as-is in case other queries want the default to be something else.

+ if response.message.startswith("Unrecognized option: Unknown option '"):
+ key = response.message[37:response.message.find("\'", 37)]
+ raise stem.socket.InvalidArguments(response.code, response.message, [key])

What happens if there are multiple unknown options? Do we then fall through to raising a InvalidRequest? What sorts of issues did you see with a 513 and 553 status?

+all = [
+ "control_message",
+ "control_line",
+ "getinfo",
+ "getconf",
+ "protocolinfo",
+ "authchallenge",
+ "singleline"
+]

Good change. Mind changing the other init.py files too?

comment:6 in reply to: ↑ 4 Changed 5 years ago by neena

Replying to atagar:

Hi Ravi. Looks pretty good, though I think that arm had a better api for this. The SETCONF and RESETCONF do the exact same thing except for undefined values (the two controller methods would probably be combined if tor could break backward comparability). Stem's Controller shouldn't blindly mirror the control-spec if we can do better. ;)

I think having seperate set_conf & reset_conf would be better. Even if both the methods were combined, it would be neater to be able to do

controller.reset_conf('this')

instead of

controller.set_conf('this', None, True)

Ideally, developers will be using the reset_conf method to reset configuration options only. I was on the fence about letting reset_conf also modify configuration values, because of what you mentioned (it does the same thing), but then that would make it impossible to do "RESETCONF x=y".

comment:7 follow-up: Changed 5 years ago by atagar

  • Status changed from needs_review to needs_information

comment:8 in reply to: ↑ 5 Changed 5 years ago by neena

Replying to atagar:

What happens if there are multiple unknown options? Do we then fall through to raising a InvalidRequest? What sorts of issues did you see with a 513 and 553 status?

setconf test1 test2 test3
552 Unrecognized option: Unknown option 'test1'.  Failing.

We'll raise InvalidArguments with just the first invalid argument in exception.arguments.
Some examples...

setconf hiddenserviceport="2839 232.23.2.32:7832" hiddenservicedir="/tmp"
513 Unacceptable option value: Failed to configure rendezvous options. See logs for details.
setconf dirport=abcd
513 Unacceptable option value: Invalid DirPort/DirListenAddress configuration
setconf disabledebuggerattachment=0
553 Transition not allowed: While Tor is running, disabling DisableDebuggerAttachment is not allowed.

Good change. Mind changing the other init.py files too?

After this is merged.

comment:9 in reply to: ↑ 7 Changed 5 years ago by neena

  • Status changed from needs_information to needs_review

Replying to atagar:

Ok, you've persuaded me. How about this as a compromise?
https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/bc723d41ea2bdfa11ec19f301cdb4095d33eb4ff

Looks good to me.

comment:10 Changed 5 years ago by atagar

  • Resolution set to implemented
  • Status changed from needs_review to closed

Feature branch pushed. Thanks!

Note: See TracTickets for help on using tickets.