"Skipping obsolete configuration option." doesn't say which one

[arma]

In Tor 0.3.5.x:

app/config/confparse.c:
log_warn(LD_CONFIG, "Skipping obsolete configuration option '%s'", c->key);

But in 0.4.2.3-alpha:

lib/confmgt/type_defs.c:
log_warn(LD_GENERAL, "Skipping obsolete configuration option.");

Resulting in log messages like:

Oct 25 16:19:28.906 [warn] Skipping obsolete configuration option.
Oct 25 16:19:28.906 [warn] Skipping obsolete configuration option.

which are not as helpful as they could be.

The bug went in with commit c60a85d2, which I think first went into 0.4.2.1-alpha.

The new file has a XXXX next to this line:

  // XXXX move this to a higher level, once such a level exists.
  log_warn(LD_GENERAL, "Skipping obsolete configuration option.");

but I see that most tickets like #29211 and #30866 are scheduled for 0.4.3.x, and this is an 0.4.2 bug so I am filing a separate ticket.

Bug reported by OFFShare in irc.

[teor]

This is a bug on #30864, which was sponsor 31, so it is a must-have for Sponsor 31.

[teor]

nickm, can you help me work out how to do this fix?
(Or, if it's a quick one, feel free to do it.)

If we want to warn about obsolete config options using their names (key), we need to pass config_line_t to parse(), or pass key and value to parse.

Which design should we use for 0.4.2?
Should we change it in 0.4.3?

[nickm]

My suggestion would be to change how OBSOLETE is implemented: it doesn't really make sense as a type. Instead, there should be an "empty" type for fields that can't be read or written, and a CFLG_OBSOLETE flag that causes these warnings. The CFLG_OBSOLETE flag should be handled at the confvar layer, I think.

This makes more sense for 0.4.3 than 0.4.2, though. Personally I think it would almost be okay to leave this as-is in 0.4.2, since we haven't made any options obsolete that weren't obsolete before. But if we think we really need a temporary fix there, we could pass the name down to the typedvar layer in any kludgey way we want.

[teor]

It's a serious ux issue for people upgrading from a much earlier version to 0.4.2.
I'll work out some way to pass the name or config line down.

And I'll open another ticket for the proper fix in 0.4.3.

[teor]

The 0.4.3 ticket is #32404.

[teor]

Replying to nickm:

Personally I think it would almost be okay to leave this as-is in 0.4.2, since we haven't made any options obsolete that weren't obsolete before.

We obsoleted RecommendedPackages in 0.4.2.1-alpha, but that only affects directory authority operators. (And no-one was using that option, which is why we obsoleted it.)

[teor]

See my PRs:

• 0.4.2: ​https://github.com/torproject/tor/pull/1521

Revert the code changes, and use #32404 instead for master:

• ​https://github.com/torproject/tor/pull/1523

Edit: I'll do extra tests that depend on #32295 and #32404 in a separate branch.

[nickm]

Looks correct. #32404 is already merged to master. Please merge as appropriate.

[teor]

Merged the code changes to 0.4.2, and then merged forward with an "ours" merge to master, to prefer the changes in #32404.

Cherry-picked the new tests to 0.4.2, fixed the commit message, and then did a standard merge forward to master.