config refactoring: fix hierarchy of configuration variable flags
From #31494 (moved), previously from comments on #30935 (moved):
- Convert the "contained" flag to something more like "NOCOPY NOCHECK NODUMP".
- at the higher level, split "contained" into "derived" and "obsolete". At the lower level, give both "derived" and "obsolete" the flags "NOCOPY NOCHECK NODUMP".
- even though these concepts may have the same flags right now, we don't want to lock them in to having the same flags in future. Because they are separate concepts that are quite different. For example, "derived" has a (derived) value, but "obsolete" has no value.
- Make the "invisible" flag more like "NODUMP NOREAD".
- Make sure that all low-level flags are orthogonal.
- Make sure that "invisible" vs "hidden" is more clear.
Activity
changed milestone to %Tor: 0.4.2.x-final
Trac:
Parent Ticket: #29211 (moved)
Here's a proposed design -- what do you think?
== The current situation ==
Currently we have two kinds of flags:
- flags for types
- flags for variables.
If a flag is set on a type, it applies to every variable of that type. If a flag is set on a variable, it applies only to that variable.
The type flags are:
unsettable -- cannot be set directly by name. (LINELIST_V, OBSOLETE)
contained -- addresses part of another type. (LINELIST_S, OBSOLETE)
cumulative -- setting a variable of this type does not override older values set to this type. (all LINELIST, LINELIST_V, LINELIST_S)
The variable flags are:
invisible -- does not show up on lists of variables, does not get written to disk, and is not visible to the controller.
obsolete -- produce a warning on any attempt to set or fetch the option. Do not list it as a valid option.
nodump -- do not write to disk. These are mostly testing options.
The refactoring
The new orthogonal low-level options are:
-
NOSET -- cannot be set by name.
-
NOLIST -- does not appear in lists of options.
-
NODUMP -- do not dump this option to disk from config_dump() -- either because it is a testing option, or because it is contained in another option.
-
NOCOPY -- do not try to copy this option in config_dup, because it is contained in another option, or has no storage.
-
NOGET -- cannot be fetched by the controller.
-
CUMULATIVE -- remains unchanged. We might call it NOREPLACE or NOOVERWRITE if that's clearer?
With this set of options:
"cumulative" remains CUMULATIVE.
"nodump" remains "NODUMP".
"unsettable" becomes NOSET.
"contained" becomes NODUMP + NOCOPY
"invisible" becomes NOGET + NOSET + NODUMP + NOLIST
"obsolete" becomes NOGET + NOSET + NODUMP + NOCOPY + NOLIST.
How flags are set
Flags can be set either on a type or on a variable. Variable flags are or'd with the flags of their type before checking them.
Replying to nickm:
Here's a proposed design -- what do you think?
Overall, it looks good.
I'd like this documentation to turn into module comment(s) in the appropriate module(s).
I have some suggestions about names. But naming is hard, so I am happy to go with whatever you decide.
== The current situation ==
Currently we have two kinds of flags:
- flags for types
- flags for variables.
If a flag is set on a type, it applies to every variable of that type. If a flag is set on a variable, it applies only to that variable.
I hadn't really noticed this, it's helpful to have it explained.
The type flags are:
unsettable -- cannot be set directly by name. (LINELIST_V, OBSOLETE)
contained -- addresses part of another type. (LINELIST_S, OBSOLETE)
Can we renamed "contained" to "derived"?
I am still confused when I read "contained", because it means a lot of different things, particularly when we have a containers module.
cumulative -- setting a variable of this type does not override older values set to this type. (all LINELIST, LINELIST_V, LINELIST_S)
In the man page, we say "append", I think that's a clearer name. But the grammar of "is_append" also feels weird to me. "is_appended"? So I don't have a strong opinion either way.
The variable flags are:
invisible -- does not show up on lists of variables, does not get written to disk, and is not visible to the controller.
obsolete -- produce a warning on any attempt to set or fetch the option. Do not list it as a valid option.
What does setting or fetching an obsolete option do? Nothing?
nodump -- do not write to disk. These are mostly testing options.
== The refactoring ==
The new orthogonal low-level options are:
In general, I like this set of names. But negatives are hard: most of these options start with "NO".
There's a design conflict here:
- we want the defaults to be 0x0000
- we want the option names to be easy to understand
I think this design's use of "NO" is a good compromise. But if you can think of a better one, I would be happy to review it.
-
NOSET -- cannot be set by name.
-
NOLIST -- does not appear in lists of options.
-
NODUMP -- do not dump this option to disk from config_dump() -- either because it is a testing option, or because it is contained in another option.
derived from another option?
- NOCOPY -- do not try to copy this option in config_dup, because it is contained in another option, or has no storage.
derived from another option?
-
NOGET -- cannot be fetched by the controller.
-
CUMULATIVE -- remains unchanged. We might call it NOREPLACE or NOOVERWRITE if that's clearer?
I like NOREPLACE for consistency with the other options: it's weird to have a single option that is not inverted.
With this set of options:
"cumulative" remains CUMULATIVE.
Might be renamed to "append" / NOREPLACE.
"nodump" remains "NODUMP".
"unsettable" becomes NOSET.
"contained" becomes NODUMP + NOCOPY
"contained" is renamed to "derived", and becomes NODUMP + NOCOPY
"invisible" becomes NOGET + NOSET + NODUMP + NOLIST
"obsolete" becomes NOGET + NOSET + NODUMP + NOCOPY + NOLIST.
== How flags are set ==
Flags can be set either on a type or on a variable. Variable flags are or'd with the flags of their type before checking them.
How do the struct_var_is_* functions change? Do we make matching changes in both the typed var and struct var modules?
I also opened #31637 (moved) to check that we have test coverage for:
- Option, +Option, and /Option,
- across command-line, torrc, and defaults, since we're changing the "cumulative" implementation.
-
I'm afraid this one is a larger branch than I thought it would be, but the individual commits should be very simple. (The overall diff is not
too smalltoo big either). Please let me know if you would like me to split it up.The branch is currently divided into these parts:
- Change the accessor functions in confparse.c so that they correspond to the low-level properties that we want to check.
config: replace config_var_is_cumulative with is_replaced_on_set() config: Invert sense of _is_invisible, and rename to is_listable() Document config_var_is_dumpable and config_var_is_settable. config: Introduce the concept of an "ungettable" variable. config: rename "contained" to "derived", and explain it better. config: make config_var_is_dumpable static.- Change the booleans in var_type_def_t into bit flags:
struct_var: refactor struct_var_is*() functions to delegate typed_var: Make flags into an unsigned OR of bits. Move VTFLAG_* declarations to conftypes.h Re-number VTFLAG_* values so they don't conflict with CVFLAG_*- Replace the old flags with the new ones:
Replace low-level {var_type,struct_var}_is_*() with flag inspection confparse, conftypes: Replace flags with their new names. Remove all VTFLAG_* usage. Remove all CVFLAG_* usage. Revise documentation on CFLG_* flags Changes file for 31625 (config flag refactor)Right now I am running it on CI to see what the coverage looks like.
-
I found some infelicities in our existing code while working on this ticket. I'm tracking them as:
- #31647 (moved) (There are no ungettable options)
- #31654 (moved) (differences between --list-torrc-options and GETCONF info/names)
I couldn't find a branch name or pull request number in the ticket comments, so I reviewed this pull request:
Replying to nickm:
The branch is currently divided into these parts:
- Change the accessor functions in confparse.c so that they correspond to the low-level properties that we want to check. {{{ config: replace config_var_is_cumulative with is_replaced_on_set() config: Invert sense of _is_invisible, and rename to is_listable() Document config_var_is_dumpable and config_var_is_settable. config: Introduce the concept of an "ungettable" variable. config: rename "contained" to "derived", and explain it better. config: make config_var_is_dumpable static. }}}
I did a review of this part, I expect to review the other parts tomorrow or Thursday (within 48 hours).
Trac:
Actualpoints: .6 to .8
Status: needs_review to needs_revision-
I've tried to make the requested changes where I could. (There's one case where I'd like to use a different ticket #31654 (moved) since there's a behavioral change involved, and there are other cases where the code in question is removed later in the branch.)
I think I've responded to all of your comments, but please let me know if I missed anything. I've tested the squash operation, and the branch squashes cleanly.
I think the final design isn't quite what was written in the ticket, but I still think it's ok.
Yeah, my apologies there -- the original list of flags wasn't quite right to match Tor's current behavior, and I wanted to avoid any behavioral changes on this branch.
The code is mostly ok, but does need a few more comments, and maybe some tidying up.
Do you have any tidying in mind? I didn't see any requests of this form, but I'm happy to give it a try.
Trac:
Status: needs_revision to needs_review Replying to nickm:
The code is mostly ok, but does need a few more comments, and maybe some tidying up.
Do you have any tidying in mind? I didn't see any requests of this form, but I'm happy to give it a try.
I think I meant "comment tidying up", I can't see anything else that needs doing.
Trac:
Status: needs_review to merge_ready
Sponsor: N/A to Sponsor31-must
Actualpoints: .8 to .9mentioned in issue #31706 (moved)
mentioned in issue #29211 (moved)
moved to tpo/core/tor#31625 (closed)
mentioned in issue tpo/core/tor#31706 (closed)
