Opened 4 months ago

Closed 2 months ago

#21663 closed task (implemented)

prop278: Refactor the torgzip module to support additional compression schemes

Reported by: ahf Owned by: ahf
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201703, prop278
Cc: Actual Points:
Parent ID: Points: 3
Reviewer: Sponsor: Sponsor4

Description

The current torgzip module should be refactored such that the new compression schemes needed for prop#278 can fit nicely into the code.

This is the tracking bug for this task.

Child Tickets

TicketSummaryOwner
#21664prop278: Make the current 'torgzip' module a submodule of a new 'compression' moduleahf

Change History (12)

comment:1 Changed 4 months ago by ahf

  • Owner set to ahf
  • Status changed from new to accepted

comment:2 Changed 4 months ago by ahf

  • Actual Points 3 deleted
  • Points set to 3

comment:3 Changed 4 months ago by ahf

  • Keywords TorCoreTeam201703 added

comment:4 Changed 4 months ago by ahf

  • Keywords prop278 added

Add prop278 keyword.

comment:5 Changed 2 months ago by ahf

Starting to make my big zstd/xz branch reviewable in https://gitlab.com/ahf/tor/compare/master...prop278%2F21663

Still a few things missing before I mark this as needs_review, but this will make it easier for other people to track the progress here.

comment:6 Changed 2 months ago by nickm

Awesome! Is the list of missing features recorded somewhere? That will help distinguish "absent but planned" from "absent and not planned"

comment:7 Changed 2 months ago by ahf

The patches in my prop278/21663 branch is the cleaned up versions of the patches in bugs/21663 branch which is currently a bit of a mess.

The only part missing in the bugs/21663 branch is currently the streaming code for Zstd/LZMA, but the tor_compress() and tor_uncompress() functionality is working (and passing tests).

I think it's realistic that this bug will be marked needs_review tonight (Danish time) when I'm done splitting things up into something that is reviewable, which will include patches for #21664 and #21662.

#21665 should be a very short fix, but is not included in the bugs/21663 branch currently; we currently pass UINT64_MAX to disable the upper bound of memory consumption for LZMA.

Last edited 2 months ago by ahf (previous) (diff)

comment:8 Changed 2 months ago by nickm

I looked it over, and what's there looked fine, but I didn't see any actual lzma2 or zstd support. That's coming in the streaming code? If so I'll have another look later in the day

comment:9 Changed 2 months ago by ahf

  • Status changed from accepted to needs_review

Refactoring and clean-up code for this issue and issue #21664 can be found in:

https://gitlab.com/ahf/tor/merge_requests/1/commits

comment:10 Changed 2 months ago by ahf

The Zstd/LZMA code is in the big messy branch (bugs/21663) which I'm turning into smaller reviewable patches in the respective issues here on trac.

comment:11 Changed 2 months ago by nickm

  • Status changed from needs_review to needs_revision

Looks pretty good to me! The only issues I noted on the ticket were pretty minor.

comment:12 Changed 2 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_revision to closed

Merging!

Note: See TracTickets for help on using tickets.