Opened 3 years ago

Closed 3 years ago

#21667 closed task (implemented)

Prop278: Handle new headers in directory.c

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: 2
Reviewer: Sponsor: Sponsor4

Description

Handle the newly defined headers and their new values from Prop#278 in the directory server/client code.

Child Tickets

TicketStatusOwnerSummaryComponent
#21668closedahfProp278: Update cached_dir_t and related types to no longer assume single compresison methodCore Tor/Tor
#22065closednickmprop278: Parse the Accept-Encoding header and pass it to "get" handlersCore Tor/Tor

Change History (19)

comment:1 Changed 3 years ago by ahf

Milestone: Tor: 0.3.1.x-final

comment:2 Changed 3 years ago by ahf

Keywords: TorCoreTeam201703 added

comment:3 Changed 3 years ago by ahf

Owner: set to ahf
Status: newaccepted

comment:4 Changed 3 years ago by ahf

Keywords: prop278 added

Add prop278 keyword.

comment:5 Changed 3 years ago by nickm

Part of this is partially done in my parse_accept_encoding branch. That only handles the parsing: it doesn't actually do anything with compression types other than deflate. I'll add some tests for it when I get back to it.

comment:6 Changed 3 years ago by ahf

comment:7 Changed 3 years ago by ahf

Status: acceptedneeds_review

I think I could use some feedback on this now: https://gitlab.com/ahf/tor/merge_requests/11?expand_all_diffs=1

I'm missing two essentials: lift my usage of srv_meth_pref_precompressed into a separate constant and deny LZMA during request negotiation for the dynamic endpoints. I'll add these changes as fixups.

comment:8 Changed 3 years ago by nickm

Status: needs_reviewneeds_revision

I added some comments there, but one thing I don't understand -- when does this ever actually serve compressed consensus documents correctly? It changes streaming compression, but it doesn't serve the right spooled_resource_t objects for cached consensuses. Is that coming in a separate commit?

comment:9 Changed 3 years ago by ahf

That will arrive in a separate commit; working on that right now while having pulled your changes from the compress_consensus_more_ways branch in.

Do you agree on having a variable like srv_meth_pref_precompressed where we define the available order, excluding LZMA, for non-precompressed documents?

comment:10 Changed 3 years ago by nickm

Yes, that sounds good. Maybe srv_meth_pref_streaming ?

comment:11 Changed 3 years ago by ahf

Works for me.

comment:12 Changed 3 years ago by ahf

The code from bug #21668 and the changes already reviewed in PR11 on Gitlab have been moved into https://gitlab.com/ahf/tor/merge_requests/12/commits

Currently tracing a bug when running this code in chutney where we initially receive a ZLIB compressed object, which we claim is compressed with LZMA.

One of the patches contains a new API in consdiff manager to query metadata around a consensus document. These new functions are currently just stubs as per discussion on IRC tonight.

comment:13 Changed 3 years ago by nickm

I have made a branch 21667_2_plus in my public repository to fill in the blank API functions. I also had to change them a bit; I made corresponding changes to the callers. Untested; sorry!

comment:14 Changed 3 years ago by nickm

I also left some comments on the Gitlab PR. Enjoy!

comment:15 Changed 3 years ago by ahf

Status: needs_revisionneeds_review

Patches added to GL - your comments should be addressed as well.

5 of the dir_handle_get/... tests are currently failing, looking into that now.

comment:16 Changed 3 years ago by nickm

patches look fine -- once the tests are working, let's merge.

comment:17 Changed 3 years ago by nickm

My branch 21667_2_testing fixes 2 of the failing tests, and adds XXXX comments explaining how to fix the other 3

comment:18 Changed 3 years ago by ahf

Patch to fix test execution landed. Not exactly as per the instructions since a lot of the code in handle_get_current_consensus() does various NULL checks for cached_consensus, so we need to inject a consensus anyway - unless I'm overseeing something.

comment:19 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Squashed and merged. Wooo!

Note: See TracTickets for help on using tickets.