Cell handling code duplication in channel.c

In 336c856e52d211aad6b40d29986264f3277a1327 :

• channel_get_var_cell_handler() and channel_get_cell_handler() looks very similar
• channel_write_cell(), channel_write_packed_cell() and channel_write_var_cell() is mostly duplicated code.
• so are channel_queue_cell() and channel_queue_var_cell()

We should refactor to reduce the code duplication.

changed milestone to %Tor: 0.3.0.x-final

added component::core tor/tor easy milestone::Tor: 0.3.0.x-final owner::pingl points::0.1 priority::medium refactoring resolution::implemented reviewer::dgoulet reviewer::nickm severity::normal status::closed type::defect labels

Trac:
Status: new to accepted
Owner: N/A to rl1987

These functions as you have mentioned have same code ,, so we need to modify it . I would like to know what will be the solution of this bug as i am new to bug solving

Trac:
Username: balubenith

Probably the best way to handle this would be to figure out how and whether cell_t, var_cell_t, and packed_cell_t can (or should) be merged into a single type, and/or to look at dataflow to see if some of those functions can be rewritten to use the others instead.

One wrinkle here is that the *cell_t types need to be very efficient; every piece of data that Tor transmits goes through one of them.

As for eliminating duplicate code in general: I think it's a higher priority to do it for larger functions, and proportionally less valuable to do it for smaller functions.

Trac:
Severity: N/A to Normal
Sponsor: N/A to N/A
Points: N/A to small/medium

channel_write_cell(), channel_write_packed_cell() and channel_write_var_cell() are combined into the channel_write() function that takes as a 3rd argument the cell type. That's defined in channel.h as enum with 3 possible values: CELL, PACKED_CELL, VAR_CELL.

Trac:
Username: pingl
Reviewer: N/A to N/A

Trac:
Status: accepted to needs_review
Milestone: Tor: unspecified to Tor: 0.2.9.x-final

Trac:
Keywords: refactoring easy deleted, refactoring, easy, review-group-9 added

Trac:
Owner: rl1987 to pingl
Status: needs_review to assigned

Trac:
Status: assigned to needs_review

Hey pingl, you might consider uploading your patches on a git repo somewhere, and then post a link to the branch here. As an example, here is another ticket where this is done: https://trac.torproject.org/projects/tor/ticket/19487#comment:3

This will make review slightly easier and it's a better way to contribute code to Tor.

Thanks :)

Sorry, here it is the link for the git tree :)

https://github.com/aigna/tor/tree/defect13827

Trac:
Username: pingl

(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!

Trac:
Points: small/medium to 0.1
Reviewer: N/A to dgoulet
Status: needs_review to needs_revision

Please check the new patches. 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.

Also this is the link for the git commit https://github.com/aigna/tor/tree/defect13827/

Replying to 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!

Trac:
Username: pingl

Trac:
Username: pingl

test_channel.c.patch

test_channel.c patch

Trac:
Username: pingl

Trac:
Username: pingl

relay.c.patch

relay.c patch

Trac:
Username: pingl

Trac:
Status: needs_revision to needs_review