Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#1692 closed enhancement (fixed)

No Events for SETCONF

Reported by: atagar Owned by: krkhan
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: special@…, krkhan@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hi, to try and improve arm's performance I've introduced client side caching for relatively static configuration parameters. However, I've run into an issue for the case where other controllers tamper these values (for instance, changing options in vidalia while arm's running).

Currently there's no notifications for SETCONF calls. In particular, control_setconf_helper (or/control.c line 623) doesn't log anything except warnings in case of failure. Could successful calls be made to produce an event, perhaps at the INFO level? Thanks! -Damian

PS. I'm suspecting that for heavily loaded relays having tor provide all INFO level events just on the off chance that this'll occur is gonna cause a pretty big hit in terms of performance. If it's a problem I'll probably just listen to them if the load's low (not a good solution, but the cure is probably worse than the disease in that case).

I'd much rather avoid drinking from the fire hose just to pick up this piece of information. While the INFO and DEBUG levels have a lot of nice information controllers can use to infer things for which no GETINFO option exists, this seems likely to come at a very high price.

Do we have any metrics for the performance costs of listening to these events? If problematic, any thoughts on how this might be addressed? One option would be letting controllers provide regex filters for the events they'd like to receive.

Child Tickets

TicketTypeStatusOwnerSummary
#3724enhancementclosedkrkhanSupport CONF_CHANGED events

Attachments (9)

confChangedEvents.patch (5.5 KB) - added by atagar 7 years ago.
Tor patch to provide CONF_CHANGED events.
TorCtl.py.patch (2.6 KB) - added by atagar 7 years ago.
TorCtl patch to detect CONF_CHANGED events.
confChangedEvent.py (522 bytes) - added by atagar 7 years ago.
Test script that, with a patched TorCtl, prints CONF_CHANGED events to stdout.
0001-Adding-a-CONF_CHANGED-event-type.patch (6.2 KB) - added by atagar 7 years ago.
Changes from the first round of comments.
0001-Adding-a-CONF_CHANGED-event-type.2.patch (4.6 KB) - added by krkhan 6 years ago.
Fixed memleak by freeing each line on the smartlist, updated to use control_event_table
0002-Don-t-send-CONF_CHANGED-event-if-no-values-are-chang.patch (852 bytes) - added by krkhan 6 years ago.
Fix bug which sent empty lines in case no conf options were changed
0001-Add-support-for-multiline-replies-from-CONF_CHANGED.patch (2.9 KB) - added by krkhan 6 years ago.
Provide a {key:value} dict to event listeners for values that changed
example-conf_changed.py (705 bytes) - added by krkhan 6 years ago.
Test TorCtl's handling of CONF_CHANGED
0001-CONF_CHANGED-sans-multiline-value-escaping.patch (4.3 KB) - added by krkhan 6 years ago.
Config options with multiline values do not exist, handle_control_getinfo escapes multiline values since they're infos not configs

Download all attachments as: .zip

Change History (41)

comment:1 Changed 7 years ago by nickm

I've got no objections. I think, if there's an event for configuration changes, it should tell you all configuration changes, including those caused by HUP, not just those caused by a SETCONF.

By "produce an event at the INFO level" you mean a STATUS event? I hate to cram more stuff into those, especially stuff that isn't related to the status of the Tor server.

Or do you mean an INFO-level log? I'm fine with adding INFO or DEBUG-level messages for this, but the format of individual log messages should not be part of Tor's controller API. If there's any info that controllers need to parse from the log, that's a gap in the API, not something to emulate.

Instead, I think this ought to be a new event type, accessible via SETEVENTS.

comment:2 Changed 7 years ago by nickm

Milestone: Tor: unspecified

Throwing this into "unspecified". It's a decent idea if anybody wants to bell the cat.

comment:3 Changed 7 years ago by atagar

Owner: set to atagar
Status: newassigned

This looks like it'll be pretty simple tweak in set_options [1]. It's the common handler for the success case of both SETCONF and RELOAD/SIGHUP:

SETCONF:

  1. handle_control_setconf(conn, cmd_data_len, args) ->
  2. control_setconf_helper(conn, len, body, 0) ->
  3. options_trial_assign(lines, use_defaults, clear_first, &errstring) ->
  4. set_options(trial_options, msg) ->

SIGHUP:

  1. control_signal_act(sig) ->
  2. signal_callback(0,0,(void*)(uintptr_t)SIGHUP) ->
  3. do_hup() ->
  4. options_init_from_torrc(0, NULL) ->
  5. options_init_from_string(cf, command, command_arg, &errmsg) ->
  6. set_options(newoptions, msg)

I'll add a proposed change either after work or tomorrow morning. Cheers! -Damian

[1] https://gitweb.torproject.org/tor.git/blob/HEAD:/src/or/config.c#l645

comment:4 Changed 7 years ago by special

Cc: special@… added

Changed 7 years ago by atagar

Attachment: confChangedEvents.patch added

Tor patch to provide CONF_CHANGED events.

Changed 7 years ago by atagar

Attachment: TorCtl.py.patch added

TorCtl patch to detect CONF_CHANGED events.

Changed 7 years ago by atagar

Attachment: confChangedEvent.py added

Test script that, with a patched TorCtl, prints CONF_CHANGED events to stdout.

comment:5 Changed 7 years ago by atagar

Priority: minornormal
Status: assignedneeds_review
Version: Tor: unspecified

Hi, I've finished a patch against the current tor master for CONF_CHANGED events. These are emitted whenever a tor configuration value is changed (SETCONF and RELOAD/SIGHUP events being the common cases). The event text consists of a comma separated listing of the changed key/value pairs (just the key if the new value is undefined).

I'm also attaching the patch for TorCtl support and a little test script I used to see the events. Cheers! -Damian

comment:6 Changed 7 years ago by Sebastian

If you can, referencing a git branch somewhere or attaching a patch generated with format-patch would be great.

For review: Lacks a changes file, introduces a bunch of whitespace errors (run make check-spaces to catch those).

AttributeList looks a little ill-defined, you should probably expand it a bit to explain what an attribute is (the word attribute doesn't appear anywhere else in control spec); make sure you also explain how attribute and value are separated, what happens when the value contains a space (how is it quoted?) etc. Just giving an example will not be enough, you need to be precise in the specification.

you can replace the XXX with 0.2.3.1-alpha, if that version gets released without this patch we can still update it later.

IIRC arma told me that we don't use comments except to comment out code, so your comment should probably be wrapped in /* */.

the if (old_options) check should probably have a comment explaining that old_options isn't set during first start of Tor, and that we can't usefully send an event that early anyways.

You are also leaking memory: smartlist_join_strings(), smartlist_create() and tor_asprintf() allocate new memory that you have to free.

Just a first glance, didn't have brains for a full review yet

Changed 7 years ago by atagar

Changes from the first round of comments.

comment:7 Changed 7 years ago by atagar

Milestone: Tor: unspecifiedTor: 0.2.3.x-final

Hi Sebastian. Thanks for the help and feedback!

If you can, referencing a git branch somewhere or attaching a patch generated with format-patch would be great.

Gotcha.

Lacks a changes file

Sorry, I don't know what this means.

introduces a bunch of whitespace errors (run make check-spaces to catch those)

Where? I've tried to address the blank-lines-are-empty convention while writing and neither I, nor 'make check-spaces', are spotting them. However, it did complain about a couple lines being wide:
Wide:src/or/config.c:676
Wide:src/or/config.c:677
Wide:src/or/rephist.c:1697

I've wrapped them at eighty characters as you guys like, and left rephist.c alone.

AttributeList looks a little ill-defined...

As per this and the irc discussion I'm switching to use newline dividers. I'm not understanding the concern with respect to configuration values possibly containing newlines - if this happened then wouldn't torrc parsing and the config_dump function (which I'm basing this off) be broken? I've refined the spec description a bit too.

I'm tempted to emit an event for each config value that changes, otherwise this would seem to be the first event type that includes newlines (and this breaks TorCtl). However, doing separate events for each value would make config bundles (like hidden services) more confusing for controllers to process. Thoughts?

If we do decide to keep with a newline separated listing then I'd appreciate some suggestions from Mike for how we'll fix _read_reply (it stops processing the event at the newline, then gets confused by the following input):

File "/home/atagar/Desktop/arm/src/TorCtl/TorCtl.py", line 844, in _read_reply

raise ProtocolError("Badly formatted reply line: unknown type %r"%tp)

ProtocolError: Badly formatted reply line: unknown type 't'

what happens when the value contains a space

I don't believe that this is a concern. A space is the divider between the key and value only. If the key had spaces, then we'd need a scheme to handle this but that isn't the case.

Just giving an example will not be enough, you need to be precise in the specification.

I agree. The example was just meant to help clarify, not be a specification in itself. We don't tend to provide examples in the spec which I think is unfortunate, but if you want to take it out then that's fine.

you can replace the XXX with 0.2.3.1-alpha, if that version gets released without this patch we can still update it later.

Hm, I'm thinking that this should be filled in by the committer when being applied. Having a guessed value here risks confusion - if it's committed with XXX then it's obvious that this was a bug.

I'm setting the bug's milestone to 0.2.3.x-final since that's what nickm set my previous patch to.

IIRC arma told me that we don't use comments

Changed.

the if (old_options) check should probably have a comment explaining that old_options isn't set during first start of Tor

Added, also I was emitting an event in this case which wasn't a good idea.

You are also leaking memory: smartlist_join_strings(), smartlist_create() and tor_asprintf() allocate new memory that you have to free.

I've added a free for the elements and results. I'm pretty sure that smartlist_join_strings is freeing the temporary strings from tor_asprintf (if it isn't then we have a memory leak in config_dump too).

Cheers! -Damian

comment:8 Changed 7 years ago by atagar

Milestone: Tor: 0.2.3.x-finalTor: unspecified
Status: needs_reviewassigned

This change is incomplete. Sebastian has pointed out that there's still a memory leak with one of the attributes and the missing changes file (both easy to fix). However, there's a bigger problem in that the formatting for multiline messages isn't right. This is tersely defined in section 2.3 of the control-spec and an example for handling it can be found in the handle_control_getinfo function of 'control.c'. That said, this is looking like a bigger headache and timesink that I'm willing to put into it at this point so I'm mothballing this for now. Putting the milestone back to unspecified.

Here's the relevant irc discussion snippets in case someone else wants to pick it up:
19:59 < special> see section 2.3, MidReplyLine/DataReplyLineandEndReplyLine
20:06 < special> atagar: response lines are in the form of code[ -+] depending on if there are more lines in the response
20:07 < special> - is mid, + is data, space is end
20:07 < special> so you can do "250+XXX\r\n250 YYY"
20:10 < rransom> special: That example is incomplete -- you need a "\r\n.\r\n" after it, and the ‘Data’ would then contain the text "250 YYY".
20:11 < special> rransom: where is the '.' defined?
20:12 < special> oh, nevermind
20:13 < atagar> Then guess I'm the only one a little lost. What is the period for?
20:13 < special> atagar: it's like SMTP
20:13 < katmagic> The period is to terminate the data.
20:13 < special> 'data' is terminated by the \r\n.\r\n

comment:9 Changed 6 years ago by atagar

Owner: atagar deleted

Ack, forgot that this was still assigned to me. Returning it to the unassigned pool (or attempting to, anyway).

Changed 6 years ago by krkhan

Fixed memleak by freeing each line on the smartlist, updated to use control_event_table

comment:10 Changed 6 years ago by krkhan

Cc: krkhan@… added
Owner: set to krkhan

In case of multiple values for SETCONF, multiline replies are generated in accordance with section 2.3 of control-spec.

[krkhan@orthanc ~]$ telnet localhost 9051
Trying 127.0.0.1...
Connected to localhost.
Escape character is ']'.
authenticate ""
250 OK
setevents CONF_CHANGED
250 OK
[krkhan@orthanc ~]$ telnet localhost 9051
Trying 127.0.0.1...
Connected to localhost.
Escape character is ']'.
authenticate ""
250 OK
setconf Nickname=orthanc ExitNodes={us}
650-CONF_CHANGED 250 OK
650-ExitNodes={us}
650-Nickname=orthanc
650 OK
quit
250 closing connection
Connection closed by foreign host.
quit
250 closing connection
Connection closed by foreign host.

Changed 6 years ago by krkhan

Fix bug which sent empty lines in case no conf options were changed

Changed 6 years ago by krkhan

Provide a {key:value} dict to event listeners for values that changed

Changed 6 years ago by krkhan

Attachment: example-conf_changed.py added

Test TorCtl's handling of CONF_CHANGED

comment:11 Changed 6 years ago by krkhan

The example listener script prints a dictionary of key:value pairs for any config options that are changed via SETCONF/HUP.

Changed 6 years ago by krkhan

Config options with multiline values do not exist, handle_control_getinfo escapes multiline values since they're infos not configs

comment:12 Changed 6 years ago by krkhan

Status: assignedneeds_review

comment:13 Changed 6 years ago by nickm

The Tor patch logic looks okay, but the factoring is a little iffy for two reasons:

  • The CONTROL_PRIVATE define is only supposed to be defined for access to private members of control.h -- only control.c and unit tests are supposed to do that.
  • In general, only control.c is supposed to understand how to format controller messages, so the translation from config_line_t to a bunch of lines in a controller message should probably happen there.

Also, it needs a changes file. seems okay other than that.

comment:14 Changed 6 years ago by krkhan

I've moved the message formatting to control.c and removed the CONTROL_PRIVATE define.

comment:15 Changed 6 years ago by nickm

Better! Still looks like it needs a changes file. Also, should we be escaping any characters in values?

comment:16 Changed 6 years ago by krkhan

Added the changes file.

Since the values don't contain CRLFs, I don't think they need to be escaped.

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

Replying to krkhan:

Added the changes file.

Since the values don't contain CRLFs, I don't think they need to be escaped.

Yes, they do. (Option values might contain quotation marks, for example.) Use either esc_for_log or escaped.

Also, consider cherry-picking 4051fcf0d6bc27b51c6910ec8ff0dc704005752a and 5628256a525a6838c176b8affbea790f1c62de6f from my bug3632-debug-v2 branch (and squashing them into a single commit). smartlist_asprintf_add will make your code even more readable.

Also, please move the code that constructs a key-value list into control_event_conf_changed -- that function should take as its arguments the old and new or_options_t structures, and only make a list of the options that changed if some control-port client actually wants a CONF_CHANGED event.

Also, we don't want tab characters in Tor's source code -- in the future, please run ‘make check-spaces’ to check for them.

comment:19 in reply to:  18 Changed 6 years ago by rransom

Milestone: Tor: unspecifiedTor: 0.2.3.x-final
Version: Tor: unspecified

Replying to rransom:

Replying to krkhan:

Added the changes file.

Since the values don't contain CRLFs, I don't think they need to be escaped.

Yes, they do. (Option values might contain quotation marks, for example.) Use either esc_for_log or escaped.

Since you're passing the option values to a printf-like function, use escaped. For example:

  smartlist_asprintf_add("650-%s=%s\r\n", key, escaped(value));

Also, please move the code that constructs a key-value list into control_event_conf_changed -- that function should take as its arguments the old and new or_options_t structures, and only make a list of the options that changed if some control-port client actually wants a CONF_CHANGED event.

From control_event_circuit_status:

  if (!EVENT_IS_INTERESTING(EVENT_CIRCUIT_STATUS))
    return 0;

Your CONF_CHANGED event shouldn't happen nearly as often, but there's no point in not doing an optimization that easy. (At the very least, your code is less likely to break a user's Tor instance if it doesn't run.)

comment:20 Changed 6 years ago by krkhan

Status: needs_reviewassigned
  • Commit: (Cherrypicked) Add smartlist_[v]asprintf_add
  • Commit: fixup! Add smartlist_[v]asprintf_add
  • Commit: Use smartlist_asprintf_add() to improve readability.
  • Commit: Escape configuration values before sending them via CONF_CHANGED.
  • Commit: Return if CONF_CHANGED isn't interesting.
  • Tor patch: Complete patch

Also, please move the code that constructs a key-value list into
control_event_conf_changed -- that function should take as its
arguments the old and new or_options_t structures, and only make
a list of the options that changed if some control-port client
actually wants a CONF_CHANGED event.

  • Comparison of options is dependent on option_is_same() and get_assigned_option().
  • option_is_same() and get_assigned_option() require an argument of type const config_format_t *.
  • config_format_t is defined inside config.c, it needs to be defined in config.h along with other structs it's dependent on in order to be accessible from control.c

I have refactored the code to make this work but since this generated a mammoth changeset, I have created a separate branch for this:

Please let me know if I should continue with the refactored branch (bug1692-refactored) or the original branch (bug1692).

comment:21 Changed 6 years ago by nickm

(Did you mean to take this out of needs_review?)

comment:22 Changed 6 years ago by nickm

I think that the original branch looks closer to the right approach: just like formatting controller stuff belongs in control.c, picking apart the various pieces of or_options_t belongs on config.c. Anything that moves the various config_var_t and friends out of config.c and makes them publicly visible is IMO a total nonstarter inasmuch as it makes the code less modular, not more.

Personally, I think we should just get to a point where we can merge bug1692, and only then consider big refactorings.

To that end, one question about 7bf046ca9f7: should the && in the first test be an
?

comment:23 Changed 6 years ago by krkhan

Status: assignedneeds_review

Oh sorry, I didn't realize that assigning the bug to me took it out of "needs review".

Spot on about 7bf046ca9f7. That condition was fixed automatically in the bug1692-refactored branch so I forgot to fix the original branch.

comment:24 Changed 6 years ago by krkhan

comment:25 Changed 6 years ago by krkhan

(Putting links in one place for ease of reference.)

comment:26 Changed 6 years ago by nickm

The tor and torspec patches look okay to me now. Any last-minute blocking reasons not to merge, rransom?

comment:27 in reply to:  26 ; Changed 6 years ago by rransom

Replying to nickm:

The tor and torspec patches look okay to me now. Any last-minute blocking reasons not to merge, rransom?

No. I'd still like to see the code to make a list of the options that changed moved out of set_options someday (since it requires utility functions private to control.c, it can only be moved into a utility function there), but that refactoring can wait. Also, this shouldn't conflict with my #3457 branch (I reclaimed EVENT_LOG_OBSOLETE instead of grabbing a new event index).

comment:28 in reply to:  27 Changed 6 years ago by rransom

Replying to rransom:

Also, this shouldn't conflict with my #3457 branch (I reclaimed EVENT_LOG_OBSOLETE instead of grabbing a new event index).

Er, the code patch won't conflict. The spec patch will, but that'll be easy to fix.

comment:29 Changed 6 years ago by nickm

Merged tor patch and spec patch.

comment:30 Changed 6 years ago by krkhan

Resolution: fixed
Status: needs_reviewclosed

comment:31 Changed 5 years ago by nickm

Keywords: tor-relay added

comment:32 Changed 5 years ago by nickm

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