Opened 2 months ago

Closed 2 months 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:

Description

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 2 months ago by dmr

Status: assignedneeds_review

Ok, here's the pull request:
https://github.com/torproject/stem/pull/5
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:
b00100d73410850d245cb1d1666829748f41ad69

The main test additions are here:
15b71235d7151465047afbe10b010bd0d82cb917
9c931591a7ef4ffa2f82f3d7c970c2fcb584ae9a
dc7be2c5faa9675279a3e9da263f2975e78ef762
5f3376a1d0e569a4573d474a6dd13646beb82c8a
205536e63269de39fe2d878fefdf81c4bec786f4
05ddc71784a196bdbc6e02d60eae453d5a32c6ad

Most of the rest is refactoring or minor stuff.

comment:2 Changed 2 months ago by atagar

Resolution: implemented
Status: needs_reviewclosed

Thanks Dave! Changes look great. Pushed.

Note: See TracTickets for help on using tickets.