Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#3457 closed enhancement (fixed)

Expose more circuit state-change events to controllers

Reported by: rransom Owned by: rransom
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: small-feature tor-client
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We currently send CIRC events to controllers that request them when a circuit is created, extended by one hop, or finished. Now that we send detailed information about circuits' purposes and states in those events, we should allow controllers to ask for notification events when circuits' purposes or states change, too.

Child Tickets

Change History (19)

comment:1 Changed 9 years ago by rransom

Status: newneeds_review

See feature3457-v2 ( git://git.torproject.org/rransom/tor.git feature3457-v2 ) for an implementation. This branch contains my feature2411-v3 (see #2411) and bug3465-023-v2 (see #3465) branches.

comment:2 Changed 9 years ago by rransom

See feature3457-v2 ( git://git.torproject.org/rransom/torspec.git feature3457-v2 ) for documentation for the changes in my feature3457-v2 tor branch. This branch contains my bug2411a-v2 torspec (see #2411) branch.

comment:3 Changed 9 years ago by nickm

(Can't merge this till 2411 is in; can't merge 2411 until pytorctl and friends have been fixed for a while, OR 2411 has been changed not to break them.)

comment:4 Changed 8 years ago by mikeperry

FYI: The PyTorCtl changes to support #2411 (in #3679) appeared to mysteriously break the bw auths somehow (#4015)

comment:5 Changed 8 years ago by rransom

See feature3457-v3 ( https://git.torproject.org/rransom/tor.git feature3457-v3 ) for a branch rebased onto current master. This branch contains my feature2411-v4 branch (see #2411); bug3465-023-v2 was merged long ago.

comment:6 Changed 8 years ago by nickm

Why do we need a new event type for this? It seems like something we could do just fine with the current CIRC events. We can't add new CircStatus entries (I think), but we can just send back the previous CircStatus with the new purpose.

comment:7 in reply to:  6 Changed 8 years ago by rransom

Replying to nickm:

Why do we need a new event type for this? It seems like something we could do just fine with the current CIRC events. We can't add new CircStatus entries (I think), but we can just send back the previous CircStatus with the new purpose.

I want to indicate to the controller what caused the event to be sent. CIRC2 events are also a bit ‘louder’ than CIRC events, and will become even louderer if we add an INTRODUCE CIRC2 event type to tell controllers of a hidden service when an INTRODUCE2 cell has been received.

We can't add new CircStatus values for CIRC events because (a) the spec doesn't specifically allow it, and (b) Vidalia won't handle them sensibly (and I don't know how any controller could). Adding a new event type seemed much better.

comment:8 Changed 8 years ago by nickm

Keywords: small-feature added

comment:9 Changed 8 years ago by nickm

notes to myself for revisions to make:

  • rename circuit_change_purpose to circuit_set_purpose for consistency
  • Use smartlist_add_asprintf() in circuit_describe_status_for_controller
  • Come up with a better name for circ2 if possible
  • nuke the explicit enumeration-value setting in circuit_status_2_event_t
  • For formatting time, "sec,usec" is pretty horrible and not what we do elsewhere in tor or in control-spec. Either ISOTime or some spaceless variant thereupon would be better. (in circuit_describe_status_for_controller()). If for some reason we can't use ISOTime (or some spaceless variant etc), then let's use an actual decimal fraction.
  • void* arg2 is making me uncomfortable. If that's only a timeval for now, let's make it a timeval. If it really has to be lots of other things, let's make typechecking wrapper functions as needed.

rransom, do you have any suggestions for a better name for CIRC2?

comment:10 in reply to:  9 ; Changed 8 years ago by rransom

Replying to nickm:

notes to myself for revisions to make:

  • rename circuit_change_purpose to circuit_set_purpose for consistency

circuit_change_purpose isn't called (and shouldn't be) the first time a circuit's purpose is set.

  • Use smartlist_add_asprintf() in circuit_describe_status_for_controller
  • Come up with a better name for circ2 if possible
  • nuke the explicit enumeration-value setting in circuit_status_2_event_t
  • For formatting time, "sec,usec" is pretty horrible and not what we do elsewhere in tor or in control-spec. Either ISOTime or some spaceless variant thereupon would be better. (in circuit_describe_status_for_controller()). If for some reason we can't use ISOTime (or some spaceless variant etc), then let's use an actual decimal fraction.

Torperf uses that format. I agree that it sucks, but I didn't know of any formatting functions already in Tor that would emit all the information in that time struct.

  • void* arg2 is making me uncomfortable. If that's only a timeval for now, let's make it a timeval. If it really has to be lots of other things, let's make typechecking wrapper functions as needed.

rransom, do you have any suggestions for a better name for CIRC2?

CIRC_MINOR? CIRC_MINOR_EVENT?

comment:11 in reply to:  10 ; Changed 8 years ago by nickm

Replying to rransom:

Torperf uses that format. I agree that it sucks, but I didn't know of any formatting functions already in Tor that would emit all the information in that time struct.

How about ISOTime with the space replaced by a slash? eg, 2011-01-11/00:02:15.324128

  • void* arg2 is making me uncomfortable. If that's only a timeval for now, let's make it a timeval. If it really has to be lots of other things, let's make typechecking wrapper functions as needed.

rransom, do you have any suggestions for a better name for CIRC2?

CIRC_MINOR? CIRC_MINOR_EVENT?

CIRC_MINOR seems fine to me.

I'll do these changes tonight or tomorrow unless this is something you'd rather do.

comment:12 in reply to:  11 Changed 8 years ago by rransom

Replying to nickm:

Replying to rransom:

Torperf uses that format. I agree that it sucks, but I didn't know of any formatting functions already in Tor that would emit all the information in that time struct.

How about ISOTime with the space replaced by a slash? eg, 2011-01-11/00:02:15.324128

The standard spaceless format uses a “T” there instead.

  • void* arg2 is making me uncomfortable. If that's only a timeval for now, let's make it a timeval. If it really has to be lots of other things, let's make typechecking wrapper functions as needed.

rransom, do you have any suggestions for a better name for CIRC2?

CIRC_MINOR? CIRC_MINOR_EVENT?

CIRC_MINOR seems fine to me.

I'll do these changes tonight or tomorrow unless this is something you'd rather do.

I'm not up to writing Tor patches tonight.

comment:13 Changed 8 years ago by nickm

See branch feature3457-v4-nm in my public repository. It looks okay to me, but could probably use at least cursory review.

comment:14 in reply to:  13 Changed 8 years ago by rransom

Replying to nickm:

See branch feature3457-v4-nm in my public repository. It looks okay to me, but could probably use at least cursory review.

Looks good! (The unit tests for format_iso_time et al. are great!)

comment:15 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged feature3457-v4-nm-squashed

comment:16 Changed 8 years ago by nickm

Revised and merged the torspec changes too. Please have a look at those as well

comment:17 in reply to:  16 Changed 8 years ago by rransom

Replying to nickm:

Revised and merged the torspec changes too. Please have a look at those as well

The revisions look good.

comment:18 Changed 7 years ago by nickm

Keywords: tor-client added

comment:19 Changed 7 years ago by nickm

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