No Events for SETCONF
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.
Activity
changed milestone to %Tor: 0.2.3.x-final in legacy/trac
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.
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:
- handle_control_setconf(conn, cmd_data_len, args) ->
- control_setconf_helper(conn, len, body, 0) ->
- options_trial_assign(lines, use_defaults, clear_first, &errstring) ->
- set_options(trial_options, msg) ->
SIGHUP:
- control_signal_act(sig) ->
- signal_callback(0,0,(void*)(uintptr_t)SIGHUP) ->
- do_hup() ->
- options_init_from_torrc(0, NULL) ->
- options_init_from_string(cf, command, command_arg, &errmsg) ->
- 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
Trac:
Owner: N/A to atagar
Status: new to assignedTrac:
Cc: N/A to special@dereferenced.netTrac:
confChangedEvents.patchTrac:
TorCtl.py.patchTrac:
confChangedEvent.pyHi, 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
Trac:
Priority: minor to normal
Status: assigned to needs_review
Version: N/A to Tor: unspecifiedIf 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
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
Trac:
Milestone: Tor: unspecified to Tor: 0.2.3.x-finalThis 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
Trac:
Milestone: Tor: 0.2.3.x-final to Tor: unspecified
Status: needs_review to assignedIn 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. Trac:
Cc: special@dereferenced.net to special@dereferenced.net, krkhan@inspirated.com
Owner: N/A to krkhanTrac:
example-conf_changed.pyTrac:
Status: assigned to needs_review-
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.
-
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_logorescaped.Also, consider cherry-picking 4051fcf0d6bc27b51c6910ec8ff0dc704005752a and 5628256a525a6838c176b8affbea790f1c62de6f from my bug3632-debug-v2 branch (and squashing them into a single commit).
smartlist_asprintf_addwill 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 newor_options_tstructures, and only make a list of the options that changed if some control-port client actually wants aCONF_CHANGEDevent.Also, we don't want tab characters in Tor's source code -- in the future, please run ‘
make check-spaces’ to check for them.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_logorescaped.Since you're passing the option values to a
printf-like function, useescaped. 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 newor_options_tstructures, and only make a list of the options that changed if some control-port client actually wants aCONF_CHANGEDevent.From
control_event_circuit_status:if (!EVENT_IS_INTERESTING(EVENT_CIRCUIT_STATUS)) return 0;Your
CONF_CHANGEDevent 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.)Trac:
Version: Tor: unspecified to N/A
Milestone: Tor: unspecified to Tor: 0.2.3.x-final- 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()andget_assigned_option(). -
option_is_same()andget_assigned_option()require an argument of typeconst config_format_t *. -
config_format_tis 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).
Trac:
Status: needs_review to assigned-
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 || ?
- Vidalia patch: Add support for CONG_CHANGED events.
(Putting links in one place for ease of reference.)
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_optionssomeday (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 legacy/trac#3457 (moved) branch (I reclaimedEVENT_LOG_OBSOLETEinstead of grabbing a new event index).Replying to rransom:
Also, this shouldn't conflict with my legacy/trac#3457 (moved) branch (I reclaimed
EVENT_LOG_OBSOLETEinstead of grabbing a new event index).Er, the code patch won't conflict. The spec patch will, but that'll be easy to fix.