Opened 8 years ago

Closed 7 years ago

#6153 closed defect (fixed)

Make circ_times static, and abstract most of its accessors.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: cbt easy tor-client
Cc: piet Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


The circ_times structure is exported as a global from circuitbuild.c. It really shouldn't be. It has 3 kinds of external users FWICT:

  • Things that pass it as an argument to circuit_build_*().
  • Two functions in circuituse.c that want to know the current timeout values.

For the former we could use an accessor function, or we could have the functions in circuitbuild.c use the static object if they receive NULL. For the latter, we could have a function that returns the current timeouts.

Making this change would also let us more completely avoid looking at circ_times when LearnCircuitBuildTimeout is 0.

Child Tickets

Attachments (1)

ticket6153.patch (17.0 KB) - added by piet 7 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 8 years ago by mikeperry

Be a little careful with this. We want to keep the ability to make CBT a per-guard property if we ever decide that's more accurate for some reason (like path bias issues). Nothing in the description sounds like it would substantially change that, but I could see it happening in later revisions.

comment:2 Changed 8 years ago by nickm

In fact, decoupling circ_times from the rest of the code would *help* with what you propose, since it would limit how much of the codebase would depend on the format and content of circ_times.

comment:3 Changed 8 years ago by nickm

Keywords: easy added
Priority: minornormal

comment:4 Changed 8 years ago by nickm

Keywords: tor-client added

comment:5 Changed 8 years ago by nickm

Component: Tor ClientTor

comment:6 Changed 8 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final

Changed 7 years ago by piet

Attachment: ticket6153.patch added

comment:7 Changed 7 years ago by piet

Status: newneeds_review

comment:8 Changed 7 years ago by nickm

Cc: piet added

Thanks! I've tweaked it some in branch "ticket6153" in my public repository at . Could use some review.

comment:9 Changed 7 years ago by nickm

Mike says it looks okay to him, though he grumbles about the accessor names. He didn't tell me which names to use instead though, so I'll merge this and see whether he feels strongly enough to write a patch. :)

comment:10 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged. Thanks!

Note: See TracTickets for help on using tickets.