Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ticket31240v2 merged 2 merged #1290

Merged

Conversation

Labels
None yet
Projects
None yet
3 participants
@nmathewson
Copy link
Contributor

@nmathewson nmathewson commented Sep 4, 2019

No description provided.

nmathewson added 30 commits Jul 24, 2019
Remember that our goal in the present refactoring is to allow each
subsystem to declare its own configuration structure and
variables.  To do this, each module will get its own
config_format_t, and so we'll want a different structure that wraps
several config_format_t objects.  This is a "config_mgr_t".
The eventual design here will be that multiple config_format_t
objects get registered with a single config_mgr_t.  That
config_mgr_t manages a "top-level" object, which has a pointer to
the other objects.

I had earlier thought of a different design, where there would be no
top-level object, and config_mgr_t would deal with a container
instead.  But this would require a bunch of invasive refactoring
that I don't think we should do just yet.
Iterating over this array was once a good idea, but now that we are
going to have a separate structure for each submodule's
configuration variables, we should indirect through the config_mgr_t
object.
We'll want it to check all the subsidiary structures of the
options object.
It's important to make sure that we don't change a config_mgr_t
after we start using it to make objects, or we could get into
inconsistent states.  This feature is the start of a safety
mechanism to prevent this problem.
We'll need to do it this way once the objects become more complex.
Every time we finalize a config manager, we now generate a new magic
number for it, so that we'll get an assertion failure if we ever try
to use an object with a different configuration manager than the one
that generated it.
The right way to free a config object is now to wrap config_free(),
always.  Instead of creating an alternative free function, objects
should provide an alternative clear callback to free any fields that
the configuration manager doesn't manage.

This lets us simplify our code a little, and lets us extend the
confparse.c code to manage additional fields in config_free.
Right now, it doesn't do anything; this patch is meant to make sure
that we're doing memory management correctly.
A configuration manager, in addition to a top-level format object,
may now also know about a suite of sub-formats.  Top-level
configuration objects, in turn, may now have a suite of
sub-objects.
This discovered a bug related to an extra & in
config_mgr_list_deprecated_vars(): fix that.
Fix a bug in config_find_option_name() where it did not consider
the abbreviations table.
This test case, at this point, only constructs the confmgr object.
More code to come.
One test makes sure that the toplevel magic numbers are distinct.

One test makes sure that we can parse a configuration object
with two sub-objects.
@coveralls
Copy link

@coveralls coveralls commented Sep 4, 2019

Pull Request Test Coverage Report for Build 6047

  • 334 of 363 (92.01%) changed or added relevant lines in 4 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+0.1%) to 63.055%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/app/config/confparse.c 283 285 99.3%
src/app/config/statefile.c 11 14 78.57%
src/app/config/config.c 26 50 52.0%
Files with Coverage Reduction New Missed Lines %
src/app/config/config.c 1 72.86%
Totals Coverage Status
Change from base Build 6046: 0.1%
Covered Lines: 47620
Relevant Lines: 75521

💛 - Coveralls

@torproject-pusher torproject-pusher merged commit 4bcfa28 into torproject:master Sep 4, 2019
0 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment