refactor conn_close_if_marked() in main.c

Trac:
Username: cyberpunks

changed milestone to %Tor: unspecified

added component::core tor/tor milestone::Tor: unspecified priority::medium refactor reporter::cyberpunks reviewer::dgoulet severity::normal status::needs-revision type::enhancement labels

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 (moved))

Trac:
Username: cyberpunks

Trac:
Milestone: N/A to Tor: 0.3.4.x-final
Status: new to needs_review

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: 0.3.5.x-final

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

Trac:
Type: task to enhancement

cyberpunks: This is failing make check-spaces :/

Trac:
Status: needs_review to needs_revision

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.

Trac:
Username: cyberpunks

What revision is needed here?

Trac:
Username: cyberpunks

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

Trac:
Status: needs_revision to needs_review

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.

Trac:
Milestone: Tor: 0.3.5.x-final to Tor: 0.3.6.x-final
Keywords: N/A deleted, refactor added

Trac:
Reviewer: N/A to dgoulet

One comment on the PR.

Trac:
Milestone: Tor: 0.3.6.x-final to Tor: unspecified
Status: needs_review to needs_revision

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.

Trac:
Username: cyberpunks

Trac:
Status: needs_revision to needs_review

Trac:
Milestone: Tor: unspecified to Tor: 0.4.0.x-final

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.

Trac:
Status: needs_review to needs_revision
Milestone: Tor: 0.4.0.x-final to Tor: unspecified

moved to tpo/core/tor#27417 (closed)