[tor-bugs] #30914 [Core Tor/Tor]: Move struct manipulation code out of confparse.c

[Tor Bug Tracker & Wiki]

#30914: Move struct manipulation code out of confparse.c
--------------------------+------------------------------------
 Reporter:  nickm         |          Owner:  nickm
     Type:  defect        |         Status:  needs_revision
 Priority:  Medium        |      Milestone:  Tor: 0.4.2.x-final
Component:  Core Tor/Tor  |        Version:
 Severity:  Normal        |     Resolution:
 Keywords:                |  Actual Points:  1
Parent ID:  #29211        |         Points:  1
 Reviewer:  teor          |        Sponsor:  Sponsor31-can
--------------------------+------------------------------------

Comment (by nickm):

 Replying to [comment:5 teor]:
 > I did an initial skim, and asked for some comment clarifications.

 Okay, I'll look at those and answer your questions there.  If you agree
 with the answers I can try to tweak the comments.

 > I don't know how to do a comprehensive review on a 2,300 line diff:

 Hm.  I'm not getting 2300 here -- only 1527 for "git diff
 master...ticket30914" and 1774 for "git log -p master..ticket30914".  That
 makes me think that the PR might be messed up somehow. Looking at the
 github PR, I see that it's listing lots of commits that were already
 merged to master with earlier branches:  there are only supposed to be 9
 commmits in this actual branch to review.

 What I would suggest for review is to go one commit at a time.  The
 comment:3  above on this ticket explains why each commit or group of
 commits is there, and tries to make it make sense in context.

 I have made a new branch `ticket30914_merged` to solve the merge conflict
 issue, and it looks far cleaner.  I hope it will be much more
 comprehensible.  The new PR is https://github.com/torproject/tor/pull/1244
 .

 > * Are there any parts you would like me to focus on?

 I think the most important part is the general ideas and architecture.  If
 there are bugs or deficiencies we can fix them as we go ahead, but if
 there are architectural problems I need to address them asap.

 > * Is there any way to make future pull requests smaller?
 >   * I would rather review 10 small pull requests I can keep in my head,
 than one big one I struggle to understand

 I can try!  Generally I have tried to make this stuff reviewable one
 commit at a time, in hopes that doing so would simplify matters, but if
 the merged and cleaned up branch above doesn't work for you, I should look
 into even smaller branches.

 Expectations management:  quite a few other branches have already piled up
 on this one.  We should talk about I can clean them up too, ideally on an
 August timeframe.

 > * What automated tools are we using to make sure this code is high-
 quality? Should we also run extra tools on this code?
 >   * clang scan-build or fuzzers come to mind, but perhaps there are
 other better tools

 I've been aiming for high test coverage as we go along; we got to 100%
 coverage on confparse.c right before I started working on this particular
 branch.  I've usually had little luck with scan-build, but maybe we can
 extract some utility from it.

 None of the data formats currently using confparse.c are attacker-facing,
 so I'm not super worried about fuzzing, but we should fuzz down the line
 as well.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30914#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online