[tor-bugs] #1692 [Tor Relay]: No Events for SETCONF

[Tor Bug Tracker & Wiki]

#1692: No Events for SETCONF
-------------------------+--------------------------------------------------
 Reporter:  atagar       |       Owner:  atagar            
     Type:  enhancement  |      Status:  needs_review      
 Priority:  normal       |   Milestone:  Tor: 0.2.3.x-final
Component:  Tor Relay    |     Version:  Tor: unspecified  
 Keywords:               |      Parent:                    
-------------------------+--------------------------------------------------
Changes (by atagar):

  * milestone:  Tor: unspecified => Tor: 0.2.3.x-final


Comment:

 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

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/1692#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online