[tor-bugs] #6816 [Tor]: {connection_or, channel}_flush_from_first_active_circuit() should pick a new circuit for each cell

[Tor Bug Tracker & Wiki]

#6816: {connection_or,channel}_flush_from_first_active_circuit() should pick a new
circuit for each cell
-----------------------+----------------------------------------------------
 Reporter:  andrea     |          Owner:                    
     Type:  defect     |         Status:  needs_review      
 Priority:  normal     |      Milestone:  Tor: 0.2.4.x-final
Component:  Tor        |        Version:  Tor: 0.2.4.2-alpha
 Keywords:  tor-relay  |         Parent:                    
   Points:             |   Actualpoints:                    
-----------------------+----------------------------------------------------

Comment(by nickm):

 Hi!  Again, a review in installments.  As before, please expect at least
 1/3 of the comments below to be unreasonable/offbase/already discussed on
 IRC.

 PART 1:

  * channel_set_cmux_policy_everywhere (and other functions): should arg be
 constant?

    * Should we even allow changing this while Tor is running? I don't
      think it's something we ever really want to do very badly; would
      avoiding it save us complexity?

  * The "detach all circuits early so they can find the channel" comment
    doesn't make sense to me.  How do they "find" the channel?  When?
    For what?

  * The dance we need to do when we allocate a circuitmux is a little
    bit worrisome; it seems like maybe instead we should arrange the
    API so that it gets a policy as an argument when it's constructed.

  * Does the the 'circ->marked_for_close' check belong in
    circuit_set_circid_chan_helper?  It seems that if the circuit is
    marked at that point, then the code we're about to execute would
    add it to the new channel, and mess up referential integrity, right?
      *  Similar question to circuit_set_circid_chan_helper.

  * why are these circuitmux_detach_circuit calls newly necessary in
    _circuit_mark_for_close?

  * circuitmux_flush_cells() sounds from its description in the file
    comment like it should be called circuitmux_extract_cells_to_flush() or
    something.

  * I'm not sure that the chanid part of chanid_circid_muxinfo_map_t
    makes sense.  If one circuitmux exists per channel, then it has an
    implicit chanid, right?

    * Would it make sense to have this info be information kept as part
      of the circuit, opaque to the circuit?

  * Documentation for active_circuits_(head,tail) needs to explain how
    the circuits are linked in their rings; it's IIRC nontrivial.

  * The nesting in circuitmux_detach_all_circuits is pretty gruesomely
    deep.  Can this function be split up or refactored or anything?
    There's no shame in a for loop with continues.

  * I'm a little worried about memory management issues from allocating
    this many little new objects per circuit.  I guess we'll see if
    it's a problem in practice, and if it is, we can roleplay
    accordingly.

  * I wonder why the round-robin policy should be a built-in default
    rather than just one other policy.  Would that simplify stuff?

  * circuitmux_attach_circuit feels kind of swiss-army-knife-ish: it
    behaves differently depending on whether the circuit is already
    attached, whether it has cells or not, whether we knew it had
    cells, etc.  But FWICT, we call it in only one place.  Are all of
    those situations really possible when circuitmux_attach_circuit is
    called?

  * For ewma_policy, I think that's a C99 syntax you're using for
    initializing it; we don't assume C99, alas.

  * re ewma_alloc_circ_data: In the rest of the code, the way we tell
    the compiler that an argument is unused is (void)cell_count.

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