Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#3155 closed enhancement (fixed)

Single Underscore in Option Name

Reported by: atagar Owned by: nickm
Priority: Very Low Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: easy tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Config options either start with zero or double underscores with a single exception...

>>> conn.getInfo("version")
'0.2.2.23-alpha (git-b85eb949b528f4d7)'
>>> print "\n".join(sorted(conn.getInfo("config/names", "").split("\n")))
...
WarnPlaintextPorts CommaList
WarnUnsafeSocks Boolean
_UsingTestNetworkDefaults Boolean
__AllDirActionsPrivate Boolean
__DisablePredictedCircuits Boolean
__HashedControlSessionPassword LineList
__LeaveStreamsUnattached Boolean
__ReloadTorrcOnSIGHUP Boolean

I asked about this on irc, but I'm not clear from Robert's response if this means that '_UsingTestNetworkDefaults' actually shouldn't be exposed to the controller.

08:12 < atagar> I'm finding that most private options start with a double underscore, but in one case it's a single underscore... [bit removed since trac chokes on the double underscores]
08:12 < atagar> is this a mistake?
08:13 < rransom> _UsingTestNetworkDefaults is a fake option.
08:13 < rransom> The double-underscore options are for use by controllers.
08:18 < atagar> Fake option? Not following - the single underscore has some special meaning?
08:18 < rransom> atagar: _UsingTestNetworkDefaults is clobbered within Tor every time it processes its options.
08:19 < rransom> The double-underscore options are actual options that can be set from outside Tor.
08:20 < atagar> Is it right then for _UsingTestNetworkDefaults to be presented via 'GETINFO config/names'? I'm pretty sure this makes it user setable.
08:20 < atagar> yup, it does

Feel free to close if presenting the _UsingTestNetworkDefaults option this way is intended (though I'd be a little curious then what the single underscore should signify for controllers).

Cheers! -Damian

Child Tickets

Attachments (1)

0001-Don-t-return-options-preceeded-by-triple-underscores.patch (2.5 KB) - added by meejah 8 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final
Owner: set to nickm
Status: newaccepted

comment:2 Changed 8 years ago by nickm

Resolution: wontfix
Status: acceptedclosed

The single underscore is indeed as intended; it was meant to signify "This option is very weird and doesn't work like the other options." We might want to change it later on.

comment:3 Changed 8 years ago by rransom

Keywords: easy added
Milestone: Tor: 0.2.3.x-finalTor: unspecified
Priority: minortrivial
Resolution: wontfix
Status: closedreopened
Type: defectenhancement

Someone should make this weird option's name start with three underscores, and hide all options whose names start with three underscores from controllers.

comment:4 Changed 8 years ago by meejah

Status: reopenedneeds_review

comment:5 Changed 8 years ago by rransom

Status: needs_reviewneeds_revision

_UsingTestNetworkDefaults should be renamed in a separate commit on the same Git branch. (This is one major reason that Git branches are usually preferred over patches.)

The commit which renames _UsingTestNetworkDefaults to begin with ___ will also cause Tor to not save it when it writes a config file to disk (or to the control port). This is a desirable side effect, but should be mentioned in the commit message.

The commit which renames _UsingTestNetworkDefaults will break any Tor configuration containing _UsingTestNetworkDefaults. This is undesirable; does anyone who runs a Tor test network allow a Tor controller to ‘SAVECONF’ the test Tor instances?

“preceeded” is spelled “preceded”.

You could fit the important part of the don't-tell-controllers-about-triple-underscore-options commit message on one line as: “Hide options beginning with "___" from GETINFO config/items

_UseFilteringSSLBufferevents is intended to be user-settable, and should not be hidden. Perhaps a separate branch should rename it to UseFilteringSSLBufferevents. Perhaps it should be removed now. Either way, this ticket's branch shouldn't touch it; it's a separate buglet.

The code change looks good. Thanks!

comment:6 in reply to:  5 Changed 8 years ago by rransom

Replying to rransom:

You could fit the important part of the don't-tell-controllers-about-triple-underscore-options commit message on one line as: “Hide options beginning with "___" from GETINFO config/items

s/items/names/

Oops.

comment:7 Changed 8 years ago by rransom

_UseFilteringSSLBufferevents is now #5415.

comment:8 Changed 8 years ago by meejah

How should I submit a git branch directly?

I've made all the changes above except the backward-compatibility thing -- did you want something which recognizes _UsingTestNetworkDefaults as well as the triple-underscore version?

comment:9 Changed 8 years ago by meejah

Okay, more-or-less as per #tor advice, I pushed a 3155 branch to https://github.com/meejah/tor with SHA a8bcbd55368ece8b2c07abe32b94a9427957c291

comment:10 Changed 8 years ago by meejah

Status: needs_revisionneeds_review

comment:11 Changed 7 years ago by meejah

Rebased the above branch.

comment:12 Changed 7 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.4.x-final

Looks like good stuff; marking for 0.2.4.

comment:13 Changed 7 years ago by arma

Now's a good time to put this into 0.2.4, if Nick likes it

comment:14 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I like the patches, but the branch was a little fubared. Cherry-picked them, renamed _UseFilteringSSLBufferevents, and merged to master.

comment:15 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:16 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.