Opened 5 months ago

Closed 2 months ago

#23459 closed defect (implemented)

prop224: Specialize interface of hs_circuitmap_get_rend_circ_client_side()

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, prop224-extra, refactoring, easy, 032-unreached
Cc: Actual Points:
Parent ID: Points: 0.4
Reviewer: Sponsor:


We currently use hs_circuitmap_get_rend_circ_client_side() for two reasons:
a) To proceed with the rend protocol as a client when we receive an intro ack (in handle_introduce_ack_success()).
b) To close useless rend circuits in close_or_reextend_intro_circ().

To fit these two scenarios, the function hs_circuitmap_get_rend_circ_client_side() currently returns all sorts of rend circs (established and unestablished).

We can improve the logic and semantics here by splitting into two funcs. One that returns only established circs (used for (a)), and another that retuns all kinds of circs (used for (b)).

Child Tickets

Change History (6)

comment:1 Changed 5 months ago by nickm

Keywords: 032-unreached added
Milestone: Tor: 0.3.2.x-finalTor: unspecified

Mark a large number of tickets that I do not think we will do for 0.3.2.

comment:2 Changed 3 months ago by ffmancera

Status: newneeds_review

Done! Check my github branch bug23459. I hope everything is fine!

PD: I am not sure if the change file header is the appropiate one.

Last edited 3 months ago by ffmancera (previous) (diff)

comment:3 Changed 3 months ago by asn

Great! Two more nitpicks:

  • make check-spaces complains about a wide line (more than 80 columns) in hs_client.c:943. Split that into two lines as follows:
      rend_circ = hs_circuitmap_get_established_rend_circ_client_side(rendezvous_cookie);
  • Add a period to the end of your changes file as follows: Closes ticket 23459.

Do the above changes on a small fixup commit, push it and I'll mark the ticket as merge_ready.

Thanks! :-)

comment:4 Changed 3 months ago by ffmancera

I did a rebase so there is only one commit. Thanks for your review :)

comment:5 Changed 2 months ago by asn

Milestone: Tor: unspecifiedTor: 0.3.3.x-final
Status: needs_reviewmerge_ready

Looks good to me! Marking as merge_ready.

comment:6 Changed 2 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Looks reasonable, and asn likes it ... merging to master.

Note: See TracTickets for help on using tickets.