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
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 assigned