Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#3842 closed enhancement (implemented)

Missing signal/names option

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

Description

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

Child Tickets

Attachments (6)

0001-Fix-bug3842.patch (540 bytes) - added by blackpaw 6 years ago.
0002-Really-fix-bug3842.patch (2.8 KB) - added by blackpaw 6 years ago.
0003-Remove-useless-comment.patch (1.4 KB) - added by blackpaw 6 years ago.
0004-Fix-bug-3842.patch (3.5 KB) - added by blackpaw 6 years ago.
0005-Fix-bug3842.patch (4.1 KB) - added by blackpaw 6 years ago.
0006-Fix-bug3842.patch (4.1 KB) - added by blackpaw 6 years ago.

Download all attachments as: .zip

Change History (25)

comment:1 Changed 6 years ago by rransom

Keywords: easy added

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

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

comment:2 Changed 6 years ago by nickm

Keywords: controller added
Milestone: Tor: unspecified

Changed 6 years ago by blackpaw

Attachment: 0001-Fix-bug3842.patch added

Changed 6 years ago by blackpaw

comment:3 Changed 6 years ago by blackpaw

Status: newneeds_review

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

Changed 6 years ago by blackpaw

comment:4 Changed 6 years ago by blackpaw

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

comment:5 Changed 6 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.2.4.x-final

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!

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

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

comment:7 Changed 6 years ago by blackpaw

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.

Changed 6 years ago by blackpaw

Attachment: 0004-Fix-bug-3842.patch added

comment:8 Changed 6 years ago by rransom

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.

comment:9 Changed 6 years ago by blackpaw

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

comment:10 Changed 6 years ago by atagar

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

Changed 6 years ago by blackpaw

Attachment: 0005-Fix-bug3842.patch added

comment:11 Changed 6 years ago by 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.

Changed 6 years ago by blackpaw

Attachment: 0006-Fix-bug3842.patch added

comment:12 Changed 6 years ago by blackpaw

Status: needs_reviewneeds_information

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

comment:13 Changed 6 years ago by nickm

Owner: set to nickm
Status: needs_informationaccepted

comment:14 Changed 6 years ago by nickm

Status: acceptedneeds_review

comment:15 Changed 6 years ago by nickm

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.

comment:16 in reply to:  15 Changed 6 years ago by rransom

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.

comment:17 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged into master as a6169800f83101 . Thanks!

comment:18 Changed 5 years ago by nickm

Keywords: tor-client added

comment:19 Changed 5 years ago by nickm

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