Missing signal/names option

We have GETINFO options for the recognized input of just about all the controller commands via... info/names config/names events/names features/names

The only thing missing is the SIGNAL values (ie, a "GETINFO signal/names").

changed milestone to %Tor: 0.2.4.x-final

added component::core tor/tor controller easy milestone::Tor: 0.2.4.x-final owner::nickm priority::low resolution::implemented status::closed tor-client type::enhancement labels

Looks like it's time to make the function that handles SIGNAL commands table-driven just like we made SETEVENTS table-driven a few weeks ago (#3465 (moved)).

This should be an easy patch for someone who wants to practice using grep and etags to dig through Tor's source code.

Trac:
Keywords: N/A deleted, easy added

Trac:
Milestone: N/A to Tor: unspecified
Keywords: easy deleted, easy controller added

Trac:
Username: blackpaw

0001-Fix-bug3842.patch

Trac:
Username: blackpaw

Trac:
Username: blackpaw

0002-Really-fix-bug3842.patch

Trac:
Username: blackpaw

I wrote a patch for this but accidentally made it into two separate patches. One is for the changelog and the other is for the actual code. It could probably use reviewing

Trac:
Username: blackpaw
Status: new to needs_review

Trac:
Username: blackpaw

0003-Remove-useless-comment.patch

Trac:
Username: blackpaw

I realized I left some useless comment in there, so I took that out as well. Sorry about the mess.

Trac:
Username: blackpaw

This looks reasonable; I like the table-driven approach.

I wonder whether it wouldn't make sense to have the GETINFO values constructed from the table; I'd say "not necessarily", but if not, we should add a comment to the table saying that whenever we add a new value, we need to make certain to add it to GETINFO signal/names. (Otherwise we'll forget to add new values to signal/names)

Right now Tor 0.2.3.x is in feature-freeze, so I'm marking this for merge in 0.2.4.x once that branch is open.

Thanks!

Trac:
Milestone: Tor: unspecified to Tor: 0.2.4.x-final

Replying to nickm:

This looks reasonable; I like the table-driven approach.

I wonder whether it wouldn't make sense to have the GETINFO values constructed from the table; I'd say "not necessarily", but if not, we should add a comment to the table saying that whenever we add a new value, we need to make certain to add it to GETINFO signal/names. (Otherwise we'll forget to add new values to signal/names)

Yes, the GETINFO value should be constructed from the table. (Otherwise, there's no reason to remodel the rest of the code to be table-driven.)

rransom is right, so I modified it to construct GETINFO's value from the signal table. Then I went back and wrapped the whole patch into one file, so ignore the first three files (trac won't let me remove them), and use the file that starts with 0004.

Trac:
Username: blackpaw

Trac:
Username: blackpaw

0004-Fix-bug-3842.patch

Trac:
Username: blackpaw

Nice!

I see (or don't see) one more buglet -- you didn't update the getinfo_items table, so your new code can never be called. (Nick has made that mistake, too, so it's not that bad.) Adding an entry in getinfo_items will also take care of adding “signal/names” to “info/names”.

And then you might want to test this patch, in case I forgot one of the subtly important steps in adding a new GETINFO item.

While you're looking at getinfo_items, please read the macros used to construct its contents; in particular, notice the use of ‘token pasting’ to construct function names. You probably shouldn't try to use token pasting for signal_table, but it's a good thing to know about.

How would I go about testing this? I don't really know how to use the control port directly.

Trac:
Username: blackpaw

How would I go about testing this? I don't really know how to use the control port directly.

Simplest option is to use this as your torrc... ControlPort 9051

Then run telnet, give it the 'AUTHENTICATE' command, and after that you can give it controller options (in the following example 'GETINFO version')...

atagar@morrigan:~/Desktop/stem$ telnet localhost 9051
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
AUTHENTICATE
250 OK
GETINFO version
250-version=0.2.1.30
250 OK
QUIT
250 closing connection
Connection closed by foreign host.

Thanks for the help, blackpaw! -Damian

Trac:
Username: blackpaw

0005-Fix-bug3842.patch

Trac:
Username: blackpaw

Okay, I think I finally fixed everything this time. The patch is all contained in the file starting with 0005. I tested it and it seems to work without breaking anything. Thanks for all of the help guys.

blackpaw@0verfl0w:~$ telnet localhost 9051
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
AUTHENTICATE
250 OK
GETINFO events/names
250-events/names=CIRC CIRC_MINOR STREAM ORCONN BW DEBUG INFO NOTICE WARN ERR NEWDESC ADDRMAP AUTHDIR_NEWDESCS DESCCHANGED NS STATUS_GENERAL STATUS_CLIENT STATUS_SERVER GUARD STREAM_BW CLIENTS_SEEN NEWCONSENSUS BUILDTIMEOUT_SET SIGNAL CONF_CHANGED
250 OK
GETINFO signal/names
250-signal/names=RELOAD HUP SHUTDOWN DUMP USR1 DEBUG USR2 HALT TERM INT NEWNYM CLEARDNSCACHE
250 OK
SIGNAL INT
250 OK
Connection closed by foreign host.

Trac:
Username: blackpaw

Trac:
Username: blackpaw

0006-Fix-bug3842.patch

Trac:
Username: blackpaw

rransom pointed out a spelling error and bad description, so that has been fixed and placed into the 0006 patch.

Trac:
Username: blackpaw
Status: needs_review to needs_information

Trac:
Owner: N/A to nickm
Status: needs_information to accepted

Trac:
Status: accepted to needs_review

Throwing back into needs_review. This seems ready for 0.2.4.x to me, once that opens. We'll probably want to squash the patches down to a single patch before merging.

Replying to nickm:

We'll probably want to squash the patches down to a single patch before merging.

No -- apply only the 0006-... patch.

‘make check-spaces’ will complain about the line in getinfo_items.

Merged into master as a6169800f83101 . Thanks!

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

Trac:
Keywords: easy controller deleted, easy controller tor-client added

Trac:
Component: Tor Client to Tor

closed

moved to tpo/core/tor#3842 (closed)