Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#10342 closed defect (fixed)

make Circuit param a class attribute of Transport, instead of a per-method param

Reported by: infinity0 Owned by: infinity0
Priority: Medium Milestone:
Component: Archived/Obfsproxy Version:
Severity: Keywords:
Cc: asn, david, phw Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

19:47:13 <asn> still one transport instance per incoming connection
..
19:47:41 <infinity0> it's just having circuit as a separate parameter makes it look like you might have multiple
19:47:56 <asn> separate parameter of what?
19:48:03 <infinity0> of the received* methods
..
19:48:14 <asn> you are right
19:48:20 <asn> also david415 mentioned that.
19:48:28 <infinity0> ah
19:48:38 <asn> i would accept a patch that makes circuit an element of the transport class

One of doing this would be, in BaseTransport, define handshake to do self.circuit = circuit. Possibly also rename handshake to circuitConnected, since it is a callback and you don't necessarily *need* to do a handshake.

Child Tickets

Change History (9)

comment:2 Changed 6 years ago by infinity0

Note: the patch above does not do what I originally suggested in the top post. The main reason is convenience, so that Transport implementers only need to call super-init() and not super-any_other_method.

comment:3 Changed 6 years ago by asn

Patch looks plausible. I'll need to do another pass before merging.

I'm not sure if I want to rename handshake() to circuitConnected() right now. I think I would prefer to do this after bananaphone and scramblesuit get merged. David and Philipp, what do you think? If we do this change, you will also need to change your codebase to reflect that.

In any case the handshake()->circuitconnected() rename should be a commit of its own. And we should probably also mention it in the ChangeLog.

Thanks for the code :)

comment:4 Changed 6 years ago by asn

Cc: phw added

comment:5 Changed 6 years ago by infinity0

Ok, I'll split the commits. However, even without the rename, scramblesuit/bananaphone will need to update to reflect the new method signatures. It is pretty minor, though, just change circuit to self.circuit.

comment:6 in reply to:  5 Changed 6 years ago by asn

Replying to infinity0:

Ok, I'll split the commits. However, even without the rename, scramblesuit/bananaphone will need to update to reflect the new method signatures. It is pretty minor, though, just change circuit to self.circuit.

You are right. Please split the commits, and I will speak with Philipp and David so that they update their codebase.

comment:7 Changed 6 years ago by phw

I'm OK with this change. After the merge, I will update my code accordingly.

comment:8 Changed 6 years ago by infinity0

The commits have been split, I updated the branch pointer so it's on the same link as above.

comment:9 Changed 6 years ago by asn

Resolution: fixed
Status: needs_reviewclosed

Merged and pushed! Thanks for the code!

(I also fixed a small bug when calling the __init__ of the base transport (since it takes no arguments). Please run the integration tests when you apply a change. You run them by doing $ python obfsproxy/test/tester.py

Closing ticket.
Please reopen if disatisfied.

Last edited 6 years ago by asn (previous) (diff)
Note: See TracTickets for help on using tickets.