[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #30914 [Core Tor/Tor]: Move struct manipulation code out of confparse.c
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs