Opened 6 weeks ago

Closed 5 weeks ago

#31625 closed defect (fixed)

config refactoring: fix hierarchy of configuration variable flags

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-august, 042-should, dgoulet-merge
Cc: nickm, teor, gaba Actual Points: .9
Parent ID: #29211 Points: .5
Reviewer: teor Sponsor: Sponsor31-must

Description

From #31494, previously from comments on #30935:

  • 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.

Child Tickets

Change History (19)

comment:1 Changed 6 weeks ago by nickm

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.

comment:2 Changed 6 weeks ago by nickm

Reviewer: teor
Status: assignedneeds_review

comment:3 Changed 6 weeks ago by nickm

(I've set this to needs_review : there is no code yet, but I'd like your opinion on the set of options I listed above before I go ahead.)

Edited to add: it's also okay if you say "just go ahead and do it, I can't look right now".

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

comment:4 Changed 6 weeks ago by teor

At the higher level, we could use is_derived() rather than is_contained().

comment:5 in reply to:  1 Changed 6 weeks ago by teor

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 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.

comment:6 Changed 6 weeks ago by teor

Status: needs_reviewneeds_revision

comment:7 Changed 6 weeks ago by nickm

Status: needs_revisionaccepted

comment:8 Changed 6 weeks ago by nickm

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 small too big either). Please let me know if you would like me to split it up.

The branch is currently divided into these parts:

  1. 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.
    
  1. 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_*
    
  1. 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.

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

comment:9 Changed 6 weeks ago by nickm

I've added two tests to try to bring the coverage back up to 100% on all the reachable new lines. I'll put this in needs_review if they pass and if the coverage looks better.

comment:10 Changed 6 weeks ago by nickm

I found some infelicities in our existing code while working on this ticket. I'm tracking them as:

  • #31647 (There are no ungettable options)
  • #31654 (differences between --list-torrc-options and GETCONF info/names)

comment:11 Changed 6 weeks ago by nickm

Status: acceptedneeds_review

I had to fix a make distcheck bug, but CI is now passing. I think this is ready for review.

comment:12 Changed 6 weeks ago by nickm

Actual Points: .6

comment:13 in reply to:  8 Changed 5 weeks ago by teor

Actual Points: .6.8
Status: needs_reviewneeds_revision

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:

  1. 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).

comment:14 Changed 5 weeks ago by teor

I reviewed the other two parts.

I think the final design isn't quite what was written in the ticket, but I still think it's ok.

The code is mostly ok, but does need a few more comments, and maybe some tidying up.

comment:15 Changed 5 weeks ago by nickm

Status: needs_revisionneeds_review

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 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.

comment:16 Changed 5 weeks ago by nickm

Keywords: 042-should added

comment:17 in reply to:  15 Changed 5 weeks ago by teor

Actual Points: .8.9
Sponsor: Sponsor31-must
Status: needs_reviewmerge_ready

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.

comment:18 Changed 5 weeks ago by nickm

Keywords: dgoulet-merge added

comment:19 Changed 5 weeks ago by dgoulet

Resolution: fixed
Status: merge_readyclosed

Merged to master.

Note: See TracTickets for help on using tickets.