Opened 3 years ago

Closed 3 years ago

#21643 closed defect (fixed)

Prop140: Extract, test, revise, and clean the diff code

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201703, prop140, review-group-17
Cc: Actual Points: 2
Parent ID: #13339 Points: 3
Reviewer: ahf Sponsor: Sponsor4

Description

Step one of getting the consensus diff support of prop140 completed is to get the diff and patch code merged. Let's do this separately from the directory code, since that code is hairier, and may need more work.

Child Tickets

TicketStatusOwnerSummaryComponent
#21677closednickmProp140: analyze diff performanceCore Tor/Tor

Change History (13)

comment:1 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 3 years ago by nickm

I've extracted the diff/patch code (and only the diff/patch code) from the rebased consdiff branch into a new branch, prop140_21643_diff_only.

comment:3 Changed 3 years ago by nickm

After some tweaks, I can measure coverage again. Coverage on consdiff.c is at 90%, which isn't bad, but we should get it higher.

comment:4 Changed 3 years ago by nickm

Okay, now the branch is up to 100% coverage.

comment:5 Changed 3 years ago by dgoulet

Keywords: prop140 added

comment:6 Changed 3 years ago by nickm

Actual Points: 2
Status: acceptedneeds_review

Also it's fuzzed and profiled now, and optimized a bit. I'll fuzz more with the new optimizations.

comment:7 Changed 3 years ago by nickm

Keywords: review-group-17 added

comment:8 Changed 3 years ago by dgoulet

Reviewer: ahf

comment:10 Changed 3 years ago by ahf

Status: needs_reviewneeds_revision

Code review is progressing on Gitlab - still a few minor things to fix there. Marking this needs_revision for now.

comment:11 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

I believe I've addressed all the gitlab comments.

comment:12 Changed 3 years ago by ahf

Status: needs_reviewmerge_ready

Looks like all GL issues have been resolved. Looks good to me.

I don't know if we have other code that could benefit from doing base64 comparison without first decoding the base64 encoded buffer and then do a memcmp(), but if we have then it could be useful to lift that code out of the consdiff specific code.

comment:13 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay -- I've squashed this and merged it into tor, and I've added the fuzzing corpora to the tor-fuzzing-corpora repository.

Note: See TracTickets for help on using tickets.