Opened 3 months ago

Closed 3 weeks ago

Last modified 3 weeks ago

#30914 closed defect (fixed)

Move struct manipulation code out of confparse.c

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: Actual Points: 1
Parent ID: #29211 Points: 1
Reviewer: teor Sponsor: Sponsor31-can

Description

After I finish the variable-manipulation code in #30864, it's time to work on moving the struct-manipulation code out of confparse.c

Child Tickets

Change History (12)

comment:1 Changed 3 months ago by nickm

Actual Points: 1
Status: assignedneeds_review

See branch ticket30914 with PR at https://github.com/torproject/tor/pull/1123 . It is based on #30864, so probably we should review that one before moving on to this.

This is one of the first inflection points of the #29211 branch: it refactors all the pieces out of confparse that are not specifically about configuration.

Last edited 8 weeks ago by nickm (previous) (diff)

comment:2 Changed 3 months ago by dgoulet

Reviewer: catalyst

comment:3 Changed 3 months ago by nickm

Partially port routerset to being a full-fledged config type again.

This patch introduces a new "routerset_type_defn" type definition for use with typed_var_t, and uses it in confparse.c. (Recall that routerset_t is not implemented in type_defs.c because it is defined in a higher-level module.) The type is still treated as a special case in the confparse.c, but that will get fixed in a later commit.

Add new "struct_var_" functions to manipulate struct fields.

This is the main new abstraction of this branch. It adds a wrapper around typed_var_* to work with fields within structures, rather than general pointers. This new type is struct_var_t -- it can describe the type of a member either as a config_type_t enum, or as a var_type_def_t pointer.

Port confparse to use struct_var in place of typed_var.

This commit changes the relevant elements of config_var_t to use struct_var_t in place of a raw (type,offset) pair, and changes the rest of confparse.c to act accordingly.

Use struct_magic_decl to verify magic numbers in config objects
Use struct_var_{copy,eq} in confparse.c.
Use structvar to find the types for config vars.

The above three commits port confparse.c and config.c to use struct_var_t in more places.

Add a function to make sure all values in a config object are ok
A few more test cases and unreachable lines

This improves test coverage, and exercises the new function introduced previously.

changes file for ticket 30914

comment:4 Changed 7 weeks ago by teor

Reviewer: catalystteor

comment:5 Changed 4 weeks ago by teor

Status: needs_reviewneeds_revision

I did an initial skim, and asked for some comment clarifications.

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

  • Are there any parts you would like me to focus on?
  • 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
  • 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

comment:6 Changed 4 weeks ago by teor

Oh, GitHub also says there are some merge conflicts with master, so let's fix those, and re-run CI.

comment:7 in reply to:  5 Changed 4 weeks ago by nickm

Replying to 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 how 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.

Last edited 4 weeks ago by nickm (previous) (diff)

comment:8 Changed 4 weeks ago by nickm

Status: needs_revisionneeds_review

comment:9 Changed 3 weeks ago by teor

Status: needs_reviewmerge_ready

This branch and the general design seem ok to me.
Thanks for cleaning it up!

It would be helpful to have a diagram showing the ideal state, and where we are in the refactor.

There is a bit of boilerplate, but there seems to be less boilerplate as we go along. It's starting to feel a bit like trunnel. Not sure if code generation would help here, or later in the process.

I left a few minor questions on the PR, feel free to fix them and then get someone else to merge. Or leave them and fix them later.

comment:10 Changed 3 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Per discussion with nickm on IRC, he was happy with the state of the branch and will fix the remaining once merged.

Everything is green here! Merged!

comment:11 Changed 3 weeks ago by nickm

(To clarify: I like and agree with the PR questions, and I'm queueing them to finish once more is caught up. We need to move some files later in the branch, so comment changes at this state are tricky.)

comment:12 Changed 3 weeks ago by nickm

I opened #31494 to track the queue of pending work here.

Note: See TracTickets for help on using tickets.