[tor-bugs] #13827 [Core Tor/Tor]: Cell handling code duplication in channel.c

[Tor Bug Tracker & Wiki]

#13827: Cell handling code duplication in channel.c
-----------------------------------------------+---------------------------
 Reporter:  rl1987                             |          Owner:  pingl
     Type:  defect                             |         Status:
                                               |  needs_revision
 Priority:  Medium                             |      Milestone:  Tor:
                                               |  0.2.9.x-final
Component:  Core Tor/Tor                       |        Version:
 Severity:  Normal                             |     Resolution:
 Keywords:  refactoring, easy, review-group-9  |  Actual Points:
Parent ID:                                     |         Points:  0.1
 Reviewer:  dgoulet                            |        Sponsor:
-----------------------------------------------+---------------------------

Comment (by pingl):

 Please check the new channel.c attached in the previous post. I think you
 are right about DG1 and also there is an issue about the fact that the
 dereferencing is not available outside the switch scope. That means that
 cell would remain of type void*. For these reasons I've moved the (still a
 bit duplicated) code into the switch cases. I'll add the default case (I
 wasn't sure how to manage it) and change the `ctype`.

 Thanks for the comments, I am new to tor.

 Replying to [comment:12 dgoulet]:
 > (I've pull your patch into `bug13827_029_01` if anyone wants to see it
 from git.tpo in my repo.)
 >
 > DG1: This worries me:
 > {{{
 > -  q.u.var.var_cell = var_cell;
 > +  q.u.fixed.cell = cell;
 > }}}
 >
 > In theory, that could work since it's a union and all cell points there
 but kind of recipe for disaster and bad semantic. What you could do is
 take a reference on the right cell member of the union while in the switch
 case and then assign it after.
 >
 > DG2: Can't you use `CELL_QUEUE_*` as the cell type?
 >
 > DG3: Few things. I would rename `ctype` to `cell_type`. The switch case
 MUST have a default branch that you could do a BUG() on and bail. Finally,
 no need for extra space between the case and the end of the function.
 >
 > Thanks for this!

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