Opened 6 years ago

Closed 5 years ago

#8546 closed enhancement (implemented)

Make a copy-able connection-config type to limit copy burden of isolation flags, etc

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, refactoring, 026-triaged-1, nickm-patch, andrea-review
Cc: rl1987@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Right now, an increasingly large number of fields and flags are duplicated between port_cfg_t, listener_connection_t, and (say) entry_connection_t. Every field we add here needs to be added to every one of those types, and needs to be explicitly copied from each to the next during construction time.

It would make this code much more maintainable if there were a type that we just copied from object to object here.

Child Tickets

Change History (9)

comment:1 Changed 5 years ago by nickm

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

comment:2 Changed 5 years ago by rl1987

Cc: rl1987@… added

comment:3 Changed 5 years ago by nickm

Keywords: 026-triaged-1 026-deferrable added

comment:4 Changed 5 years ago by nickm

Branch "bug8546" in my public repo has a fix for this, but needs a changes file. Please review?

comment:5 Changed 5 years ago by nickm

Keywords: nickm-patch added; easy 026-deferrable removed
Status: newneeds_review

comment:6 Changed 5 years ago by nickm

Keywords: andrea-review added

comment:7 Changed 5 years ago by andrea

be8dd7f6a73d919ffdecbb95f2e7e89a49bd1c97:

  • All this looks good to me

ff40663a8e4efd28ea99ec06d178dd8859cb5101:

  • Ditto, and I should learn coccinelle

dbdc5c91250f15382ba3c132d60167402aa56f68:

  • This one looks fine to me too; I presume the intent is to copy this from the listener_connection_t to the new incoming connection?

5978c2226ae6bbe3c539b86251bd7d9afe8f82dc:

  • Looks fine

5d58a096713a8c8622d56d60b5aebd5d23616299:

  • This looks okay to me

bb48b1d03b7d8d294f7fb7aecae6bd527b5e1a92:

  • Looks good

73f17c32b2f0e39f06067ee61871f5dd9cdefaba:

  • Looks good to me

cf5d435ecc9a1918b4143d9a8f6e7bcbbd2039b8:

  • Hmm, I think we're okay here, but it looks like in some cases (i.e. type == CONN_TYPE_AP_LISTENER) we're copying the whole structure instead of just a few fields in the old code. Are we certain it won't ever carry along values that will cause problems?

d669e1b51b702c62fbc3bf0f0353458af1cd86ae:

  • s/simplifification/simplification/, surely

comment:8 Changed 5 years ago by nickm

Thanks for the review! I'm 99% sure that the CONN_TYPE_AP_LISTENER case is safe. It actually copied nearly all fields before, but set just a few of them in the non-listener case.

Fixing and merging to master.

comment:9 Changed 5 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed
Type: defectenhancement
Note: See TracTickets for help on using tickets.