[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #27417 [Core Tor/Tor]: refactor conn_close_if_marked() in main.c

#27417: refactor conn_close_if_marked() in main.c
--------------------------+----------------------------------
 Reporter:  cyberpunks    |          Owner:  (none)
     Type:  enhancement   |         Status:  needs_revision
 Priority:  Medium        |      Milestone:  Tor: unspecified
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:  refactor      |  Actual Points:
Parent ID:                |         Points:
 Reviewer:  dgoulet       |        Sponsor:
--------------------------+----------------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision
 * milestone:  Tor: 0.4.0.x-final => Tor: unspecified


Comment:

 Replying to [comment:13 cyberpunks]:
 > Replying to [comment:12 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27417#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs