Opened 12 months ago

Last modified 9 months ago

#27417 needs_revision enhancement

refactor conn_close_if_marked() in main.c

Reported by: cyberpunks Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactor
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description


Child Tickets

Change History (16)

comment:1 Changed 12 months ago by cyberpunks

The branch connection-refactor1 at https://gitgud.io/onionk/tor.git has this and two other cleanups that were prerequisites to other work (the branch in #27319)

comment:2 Changed 12 months ago by nickm

Milestone: Tor: 0.3.4.x-final
Status: newneeds_review

comment:3 Changed 12 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

comment:4 Changed 12 months ago by nickm

Created a PR as https://github.com/torproject/tor/pull/309 so it gets some CI testing

comment:5 Changed 11 months ago by nickm

Type: taskenhancement

comment:6 Changed 11 months ago by mikeperry

Status: needs_reviewneeds_revision

cyberpunks: This is failing make check-spaces :/

comment:7 in reply to:  6 ; Changed 11 months ago by cyberpunks

Replying to mikeperry:

This is failing make check-spaces :/

In crypto_util.c, a file the patches never touched?

master itself was failing check-spaces when that CI was run a week ago.

Last edited 11 months ago by cyberpunks (previous) (diff)

comment:8 in reply to:  7 Changed 11 months ago by cyberpunks

What revision is needed here?

comment:9 Changed 11 months ago by teor

Status: needs_revisionneeds_review

I opened a new pull request against the latest master:
https://github.com/torproject/tor/pull/359

comment:10 Changed 11 months ago by dgoulet

Keywords: refactor added
Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Not sure I feel comfortable getting this in after freeze. Merge window for 036 will open Oct 15th so I believe would be safer to get it in early there.

comment:11 Changed 10 months ago by dgoulet

Reviewer: dgoulet

comment:12 Changed 10 months ago by dgoulet

Milestone: Tor: 0.3.6.x-finalTor: unspecified
Status: needs_reviewneeds_revision

One comment on the PR.

comment:13 in reply to:  12 ; Changed 10 months ago by cyberpunks

Replying to dgoulet:

if (conn->linked_conn && retval >= 0) {
connection_start_reading_from_linked_conn(conn->linked_conn);
So here is a potential problem. With this refactor, this function can be called after the code below here that is moved into connection_flush_before_close().

The point about splitting it into a pure code movement commit and then a separate commit is well taken. Should do that.

That said, it's certain there's no behavior change, because if conn->linked_conn is non-NULL, it's impossible for connection_wants_to_flush(conn) to be true when you reach the code below's if statement, so the code below that's moved into the new function was never called when this is a linked connection. buf_move_to_buf() always reduces conn->outbuf_flushlen to zero, emptying the buffer entirely.

comment:14 Changed 9 months ago by dgoulet

Status: needs_revisionneeds_review

comment:15 Changed 9 months ago by teor

Milestone: Tor: unspecifiedTor: 0.4.0.x-final

comment:16 in reply to:  13 Changed 9 months ago by dgoulet

Milestone: Tor: 0.4.0.x-finalTor: unspecified
Status: needs_reviewneeds_revision

Replying to cyberpunks:

Replying to dgoulet:

if (conn->linked_conn && retval >= 0) {
connection_start_reading_from_linked_conn(conn->linked_conn);
So here is a potential problem. With this refactor, this function can be called after the code below here that is moved into connection_flush_before_close().

The point about splitting it into a pure code movement commit and then a separate commit is well taken. Should do that.

Without a "move" commit and "change" commit, I can't do a proper review here. The code flow is _not_ simple and this is a critical function. Please resubmit a branch with that structure?

That said, it's certain there's no behavior change, because if conn->linked_conn is non-NULL, it's impossible for connection_wants_to_flush(conn) to be true when you reach the code below's if statement, so the code below that's moved into the new function was never called when this is a linked connection.

I don't think so ... connection_flush_before_close() can return whatever results from buf_move_to_buf() that can makes it that we still wants to flush more after...

buf_move_to_buf() always reduces conn->outbuf_flushlen to zero, emptying the buffer entirely.

buf_move_to_buf() won't necessarily make outbuf_flushlen to zero, it can be capped by the buf_t datalen so this is not a guarantee.

I can't move forward without a breakdown in the commits, I still see a behavior change and if I'm mistaken then nickm can hand up confused also during upstream merge.

Note: See TracTickets for help on using tickets.