Skip to content
Snippets Groups Projects
New look: Off

Implement SETCONF/RESETCONF parsing in Stem

    • Closed Issue created by neena @neena

    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.

    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items ...

    • Activity

    • neena added component::archived/stem owner::neena priority::medium resolution::implemented status::closed type::task labels

      added component::archived/stem owner::neena priority::medium resolution::implemented status::closed type::task labels

    • N
      neena @neena ·
      Author

      Trac:
      Owner: atagar to neena
      Status: new to assigned

    • N
      neena @neena ·
      Author

      Trac:
      Status: assigned to needs_review

    • N
      neena @neena ·
      Author

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

    • A
      Damian Johnson @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?

    • A
      Damian Johnson @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?

    • N
      neena @neena ·
      Author

      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".

    • A
      Damian Johnson @atagar ·

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

      Trac:
      Status: needs_review to needs_information

    • N
      neena @neena ·
      Author

      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.

    • N
      neena @neena ·
      Author

      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.

      Trac:
      Status: needs_information to needs_review

    • A
      Damian Johnson @atagar ·

      Feature branch pushed. Thanks!

      Trac:
      Resolution: N/A to implemented
      Status: needs_review to closed

    • Trac closed

      closed

    Please register or sign in to reply