Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4971 closed enhancement (implemented)

Provide initvalue in GETINFO config/names

Reported by: chiiph Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

To handle torrc config options from Vidalia it would be useful to me to have the default values with each option printed by config/names.

Attached is a simple patch to handle this.

Child Tickets

Attachments (1)

0001-Provide-init-values-for-config-names-getinfo-control.patch (1.6 KB) - added by chiiph 8 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 8 years ago by chiiph

Status: newneeds_review

comment:2 Changed 8 years ago by rransom

Status: needs_reviewneeds_revision

This patch doesn't apply to current origin/master . Please push a Git branch instead.

Use esc_for_log to quote var->initvalue when it exists (instead of wrapping it with double quotes), and distinguish a NULL initvalue from an empty-string initvalue.

comment:3 Changed 8 years ago by rransom

Milestone: Tor: 0.2.4.x-final

comment:4 Changed 8 years ago by chiiph

Status: needs_revisionneeds_review

Ok, updated based on rransom's comments in my branch bug4971_initvalues.

comment:5 Changed 8 years ago by rransom

Status: needs_reviewneeds_revision

From the documentation comment on esc_for_log in src/or/util.c:

/** Allocate and return a new string representing the contents of <b>s</b>,
 * surrounded by quotes and using standard C escapes.

So your branch leaks initvalue.

There are at least three ways to avoid freeing the constant string "NULL". One is to tor_strdup it in an else clause. Another is to use the tor_strdup("(null)") which esc_for_log returns if its argument is NULL as your representation of a NULL initvalue. Another is to stick a second copy of the pointer which esc_for_log returns into another variable which can always be passed to tor_free (because it always holds a pointer to a temporary object or NULL).

Does anything else parse the output of GETINFO config/names? Does control-spec.txt specify its output format?

Other than that, the code change looks good.

comment:6 Changed 8 years ago by atagar

Does anything else parse the output of GETINFO config/names?

Yup, arm does.
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/cli/configPanel.py#l213
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torConfig.py#l182
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torConfig.py#l359
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torInterpretor.py#l317

I don't mind changing it, so please reassign this ticket to me after a tor patch is merged.

Does control-spec.txt specify its output format?

Yes it does.
https://gitweb.torproject.org/torspec.git/blob/HEAD:/control-spec.txt#l610

I don't like the idea of the initial value being at the end since it would then make this...

OptionName SP OptionType [ SP Documentation ][ SP InitialValue ] CRLF

Multiple optional arguments, especially when the first is defined as 'Text', means that controllers can't differentiate Documentation from the InitialValue. We could redefine the Documentation's Text type as not having quotes but from a parsing perspective... ewww.

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

Replying to atagar:

Does anything else parse the output of GETINFO config/names?

Yup, arm does.
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/cli/configPanel.py#l213
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torConfig.py#l182
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torConfig.py#l359
https://gitweb.torproject.org/arm.git/blob/HEAD:/src/util/torInterpretor.py#l317

I don't mind changing it, so please reassign this ticket to me after a tor patch is merged.

No. Any arm change should have a separate ticket.

Does control-spec.txt specify its output format?

Yes it does.
https://gitweb.torproject.org/torspec.git/blob/HEAD:/control-spec.txt#l610

I don't like the idea of the initial value being at the end since it would then make this...

OptionName SP OptionType [ SP Documentation ][ SP InitialValue ] CRLF

Multiple optional arguments, especially when the first is defined as 'Text', means that controllers can't differentiate Documentation from the InitialValue. We could redefine the Documentation's Text type as not having quotes but from a parsing perspective... ewww.

No version of Tor will support both Documentation and InitialValue. If Documentation was removed in Tor 0.2.2.x, arm (and other controllers) should simply drop support for it.

comment:8 Changed 8 years ago by nickm

It seems to me that if we want to change the format and semantics of an existing option in a way that the current spec doesn't permit, and if doing so will break current controllers, it might be better just to provide a new GETINFO key and call it something other than "config/names".

comment:9 Changed 8 years ago by meejah

Status: needs_revisionneeds_review

Here's a branch that implements a "GETINFO config/defaults" and simply doesn't list anything which has a NULL initvalue (which is a separate commit if you don't want that). This would need a torspec change too, which I can do if this is acceptable.

https://github.com/meejah/tor/commits/4971/

comment:10 Changed 8 years ago by nickm

looks good; we should merge this once 0.2.4.x is open.

Minor stuff it needs:

  • "make check-spaces" should pass.
  • Tor is indented in K&R style, so we write:
       } else if (condition) {
    

and not

   }
   else if (condition)
   {
  • We'll need a changes file, and a patch to control-spec.txt.

comment:11 Changed 8 years ago by meejah

I pushed changes for the K&R and whitespace -- don't know what was up with my emacs. Also, a change for torspec:

https://github.com/meejah/torspec/tree/4971

There is a changes/bug4971 in the branch:

https://github.com/meejah/tor/blob/4971/changes/bug4971

Is that what you mean by changes file?

comment:12 Changed 7 years ago by meejah

Rebased the above branch.

comment:13 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I tweaked it just a little more (by removing one of the redundant blocks in config.c, and splitting a long line), then squashed and merged into master, which will become Tor 0.2.4.

You can see the changes in "ticket4971" in my public repository.

Thanks again!

comment:14 Changed 7 years ago by nickm

Keywords: tor-client added

comment:15 Changed 7 years ago by nickm

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