Opened 6 years ago

Closed 5 years ago

#3724 closed enhancement (wontfix)

Support CONF_CHANGED events

Reported by: krkhan Owned by: krkhan
Priority: Medium Milestone:
Component: Torctl Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: #1692 Points:
Reviewer: Sponsor:

Description

#1692 implemented CONF_CHANGED events for Tor which get controllers can use to get notified of any configuration changes made by SETCONF/HUP. TorCtl should support CONF_CHANGED events for its listeners.

Child Tickets

Change History (9)

comment:1 Changed 6 years ago by krkhan

Status: newneeds_review

comment:2 Changed 6 years ago by robinson

I reviewed this and I have many questions.

Where is the specification for CONF_CHANGED events? I don't see it in control-spec.txt.

Would this be the first multi-line event handled by TorCtl? Otherwise, why the additions to _handle1()? Why should multi-line events be handled this way (e.g. set(codes)), rather than each line separately?

I recommend this be implemented under Daemon's rewrite of the event handler in ticket #3679.

comment:3 in reply to:  2 Changed 6 years ago by krkhan

Replying to robinson:

Where is the specification for CONF_CHANGED events? I don't see it in control-spec.txt.

CONF_CHANGED events were committed to the git repo (patch from #1697). Section 4.1.17 of control-spec.txt explain the event.

Would this be the first multi-line event handled by TorCtl? Otherwise, why the additions to _handle1()? Why should multi-line events be handled this way (e.g. set(codes)), rather than each line separately?

Yes, it is indeed the first multi-line event handled by TorCtl. set(codes) merely ensures that all lines belonging to the multi-line event have the same code. Individual lines are parsed after that.

I recommend this be implemented under Daemon's rewrite of the event handler in ticket #3679.

Damian shall be able to make a better judgment on whether multi-line event parsing should be included in #3679's fix.

comment:4 Changed 6 years ago by atagar

Damian shall be able to make a better judgment on whether multi-line event parsing should be included in #3679's fix.

On first glance the patch looks great and is fairly tangential to 3679 since it doesn't follow the positional/keyword argument pattern of most event types. It doesn't conflict and should be trivial to merge.

comment:5 Changed 6 years ago by nickm

Milestone: Tor: 0.2.3.x-final

comment:6 Changed 6 years ago by atagar

Little sad to see the milestone removed but makes sense.

This is what prevents controllers from doing client side caching when there are multiple controllers attached. I could partly work around this if we had an event or GETINFO option to figure out if there are other controllers so caching could be disabled in that use case. This is easy to figure out with netstat but we're kinda breaking that. ;)

comment:7 Changed 6 years ago by Sebastian

the milestone was removed because this is a torctl ticket, not tor

comment:8 Changed 6 years ago by atagar

doh, mixed up the tickets - sorry about that :)

comment:9 Changed 5 years ago by atagar

Resolution: wontfix
Status: needs_reviewclosed
Note: See TracTickets for help on using tickets.