Opened 6 weeks ago

Last modified 3 weeks ago

#30864 merge_ready enhancement

Move variable 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: dgoulet-merge
Cc: Actual Points: 3
Parent ID: #29211 Points: 1
Reviewer: catalyst Sponsor: Sponsor31-can

Description (last modified by nickm)

There is some code in confparse.c for messing with arbitrary variables that ought to become general-purpose and go into lib/. This is a first step towards splitting config.c into reasonable pieces.

Child Tickets

Change History (16)

comment:1 Changed 5 weeks ago by nickm

Sponsor: Sponsor31-can

comment:2 Changed 5 weeks ago by nickm

Description: modified (diff)
Summary: Move struct inspection code out of confparse.cMove variable manipulation code out of confparse.c

Changing purpose of ticket slightly: struct introspection is at a slightly higher level than lvalue manipulation.

comment:3 Changed 5 weeks ago by nickm

Status: assignedneeds_review

I have a branch called ticket30864 with a PR at https://github.com/torproject/tor/pull/1116 . It is based on #30893, so the first several commits there should be considered part of the other ticket.

comment:4 Changed 5 weeks ago by nickm

Actual Points: 2.5

comment:5 Changed 5 weeks ago by catalyst

Reviewer: catalyst

I'm looking over this briefly.

comment:6 Changed 5 weeks ago by nickm

TODO:

  • Add tests for unitparse.c.
  • Change the comment on POSINT to something like "This is indeed an int, not a unsigned int. For historical reasons, many configuration values are restricted to the range [0,INT_MAX]."

comment:7 Changed 5 weeks ago by nickm

Those changes are now done and #30893 is merged, so this branch is ready for review.

comment:8 Changed 5 weeks ago by nickm

Type: defectenhancement

comment:9 Changed 4 weeks ago by nickm

Squashed and rebased per request.

comment:10 Changed 4 weeks ago by nickm

Here is an overview of the commits in this branch:

checkSpace.pl: Allow 'bool' before a space and an open-paren
Start moving types that will be used for config vars to lib/conf
Move unit-parsing code to src/lib/confmgt
Add a function to append an existing line to a config line list.

These are preliminary refactoring, code movement, and cleanup commits to prepare the code for the next commit.

Add a "typed_var" abstraction to implement lvalue access in C.

This is the most important commit in the patch: it extracts code from confparse.c and moves it to lib/confmgt.

Further clarify our clarification about the type of POSINT
Add unit tests for the unitparse.c module.

These are leftover clarifications and coverage fixes from #30893.

comment:11 Changed 4 weeks ago by catalyst

Thanks! Overall this is looking fairly good. I'm still having trouble with the vagueness of some of the descriptions in the explanatory comments.

I started to write some line-by-line review comments on files in the pull request but found that I was having difficulty suggesting alternative text.

So instead, I figured I should try to summarize (at a high level) what I'm finding confusing, in the hopes that it will make it easier to suggest alternative text in the pull request comments:

  • It's not clear what the main header file for this interface is. Maybe it's typedvar.h?
  • In many places, words like "type" and "variable" are used in vague ways, and not necessarily to refer to C types or variables.

I kind of wish there were a high-level overview of how this works. My attempt to construct one from my analysis of the code is:

typedvar.h

  • typedvar.h is an interface for accessing objects whose type is designated at run-time. (Maybe a kind of dynamic typing? Can we call it dynamic typing?) Access includes encoding, decoding, copying, clearing, etc.
  • typedvar.h functions use void * as generic pointers to the dynamically typed objects. (Maybe some opaque pointer type would be better, but then explicit casts would be needed.)
  • var_type_def_t objects are descriptors that instruct the typedvar.h functions how to manipulate a given dynamic object.
  • Most of the typedvar.h functions take pointers to var_type_def_t objects instead of enumerated type numbers to discover how to manipulate a given dynamically typed object.
  • Some typedvar.h functions take enumerated values from config_type_t to look up which var_type_def_t object to use. This seems like a bit of a layering violation but it's probably an acceptable intermediate situation.

conftypes.h

  • conftypes.h holds the enumerated config_type_t values previously in confparse.h. typedvar.h functions use these enumerated values to look up which var_type_def_t describes a given dynamically typed object.

type_defs.c

  • type_defs.c implements most of the functions that typedvar.h will use for accessing the dynamically typed objects.
  • type_defs.c also has the lookup table (and lookup function) to look up the appropriate var_type_def_t descriptor from a type number from config_type_t.

comment:12 Changed 3 weeks ago by catalyst

Status: needs_reviewneeds_revision

Thanks! The code changes look good. I made a few minor comments on the pull request.

There's a mismatched Doxygen comment that should be corrected. There's a minor function naming comment that you may ignore if you like.

There's also a technical debt comment that I'm OK with you opening a new ticket about.

We talked about the broader documentation issues with this code over IRC. I will open a new ticket to improve those once the dust has settled from the other related work on this branch.

comment:13 Changed 3 weeks ago by nickm

Status: needs_revisionneeds_review

I think I've resolved the issues here with new commits on ticket30864; once you ack, we can merge, and move on to #30914.

Thanks for opening a ticket about naming/documentation.

comment:14 in reply to:  13 ; Changed 3 weeks ago by catalyst

Status: needs_reviewmerge_ready

Replying to nickm:

I think I've resolved the issues here with new commits on ticket30864; once you ack, we can merge, and move on to #30914.

This looks good to me now. Thanks for the revisions and for opening a new ticket on the tech debt issue.

Thanks for opening a ticket about naming/documentation.

I haven't done this yet, but plan to do so soon.

comment:15 in reply to:  14 Changed 3 weeks ago by catalyst

Replying to catalyst:

Replying to nickm:

Thanks for opening a ticket about naming/documentation.

I haven't done this yet, but plan to do so soon.

This is now #31078.

comment:16 Changed 3 weeks ago by nickm

Actual Points: 2.53
Keywords: dgoulet-merge added
Note: See TracTickets for help on using tickets.