Opened 11 days ago

Closed 11 days ago

#26766 closed defect (implemented)

Cell unused content is ignored while packing

Reported by: dmr Owned by: dmr
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Normal Keywords: client
Cc: atagar Actual Points:
Parent ID: Points:
Reviewer: atagar Sponsor:


The Cell unused attribute is populated when unpacking content, with any content that wasn't assigned to another attribute per the cell structure.

However, it currently isn't used for anything else*. It would make the most sense to allow such a Cell to be repacked to binary identicality, changing _pack() to allow this.

IRC log of discussion with atagar:

18:52 < dmr> atagar: (2) should `unused` be used in `_pack()`, prior to additional payload padding? I think it should, to allow repacking into something with the same binary value.
18:55 <+atagar> (2) agreed, it should
18:56 <+atagar> [...] Would you mind sending a patch for (2)?
19:08 <+atagar> dmr: Oh btw, if you could send a unit test along with (2) I'd appreciate it. Our tests should check that re-packing cells produces the same bytes we read.
19:27 < dmr> atagar: sounds good, and for (2) I was planning that very test set!

This ticket tracks the implementation change and the test changes. I've got a pull request coming soon!

* 'twas not intended, but I got quite a kick out of that. It's almost... well... unused. (Sigh.)

Child Tickets

Change History (2)

comment:1 Changed 11 days ago by dmr

Status: assignedneeds_review

Ok, here's the pull request:
Branch head 775bc60fb6497872b8ae00a5a8b5c3e29305d7d0.

I made a nice little --no-ff merge commit already - hopefully that helps with the summary :)

I took the opportunity to refactor a lot of the cell test cases. This should make it much easier to add further test cases for unused or link_protocol differences. It also makes it much more data-driven than before.

The main implementation change is here:

The main test additions are here:

Most of the rest is refactoring or minor stuff.

comment:2 Changed 11 days ago by atagar

Resolution: implemented
Status: needs_reviewclosed

Thanks Dave! Changes look great. Pushed.

Note: See TracTickets for help on using tickets.