Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3627 closed defect (implemented)

Split edge_connection_t into edge_connection_t and entry_connection_t

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

Description

See branch "split_entry_conn" in my public repository.

It divides off the entry-only fields from edge_connection_t, and makes a new type for them.

This isn't a material memory savings: we get less than 100 bytes saved per exit connection by moving the fields out of edge_connection_t. Rather, it's meant to make the code more intelligible by making edge_connection_t contain *only* the common edge connection fields.

Child Tickets

Change History (14)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

comment:2 Changed 8 years ago by arma

+  if (conn->type != CONN_TYPE_AP)
+    return 0;
+  entry = TO_ENTRY_CONN(conn);
+
   if (conn->type == CONN_TYPE_AP &&

the second check is redundant

comment:3 Changed 8 years ago by arma

Do you plan to make an exit_connection_t? It confuses me that edge_connection_new() now asserts that type can only be CONN_TYPE_EXIT.

comment:4 Changed 8 years ago by arma

Is it time to s/CONN_TYPE_AP/CONN_TYPE_ENTRY/g yet?

comment:5 Changed 8 years ago by nickm

the second check is redundant

Noted; will fix.

Do you plan to make an exit_connection_t? It confuses me that edge_connection_new() now asserts that type can only be CONN_TYPE_EXIT.

Only if there were a significant number of fields to move into it.

Is it time to s/CONN_TYPE_AP/CONN_TYPE_ENTRY/g yet?

Perhaps.

I'm also interested in what you think of this idea in general. Is it worth doing IYO?

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

Replying to nickm:

I'm also interested in what you think of this idea in general. Is it worth doing IYO?

This sounds like a good thing to do someday, but it will probably break the patches I'm writing to try to diagnose #3632 so we can debug your last pile of changes to this area. I think this change should wait until after #2411 and my new patches for #3632 are merged.

comment:7 Changed 8 years ago by nickm

Okay. I kinda worry that every change we make is likely to break this patch, and that merging will get unfeasible fast. But I guess we can try to get those in before too long. Also, isn't #2411 stalled?

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

Replying to nickm:

Okay. I kinda worry that every change we make is likely to break this patch, and that merging will get unfeasible fast. But I guess we can try to get those in before too long. Also, isn't #2411 stalled?

#2411 is stalled because PyTorCtl is broken and has no maintainer. Unfortunately, I see no way to debug the proposal 171 code without (an extended version of) that branch.

comment:9 Changed 8 years ago by nickm

Merged this to master. Thanks, all! (Please review the merge to make sure I didn't mess it up).

comment:10 Changed 8 years ago by arma

Hoo-ee. Big patch. I glanced over it again and didn't see anything more than I saw the first time.

(Changing AP to ENTRY occurred to me again while looking at it, since most people have no idea what an AP is at this point anyway.)

I built it and it compiles. What could go wrong? :)

comment:11 Changed 8 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

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

Replying to nickm:

Merged this to master. Thanks, all! (Please review the merge to make sure I didn't mess it up).

The merge looks good to me.

comment:13 Changed 7 years ago by nickm

Keywords: tor-client added

comment:14 Changed 7 years ago by nickm

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