Opened 7 weeks ago

Closed 5 weeks ago

#32295 closed defect (fixed)

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

Reported by: arma Owned by: teor
Priority: Medium Milestone: Tor: 0.4.2.x-final
Component: Core Tor/Tor Version: Tor: 0.4.2.1-alpha
Severity: Normal Keywords: network-team-roadmap-november, 042-backport
Cc: nickm Actual Points: 0.2
Parent ID: #29211 Points: 0.5
Reviewer: nickm Sponsor: Sponsor31-must

Description

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.

Child Tickets

Change History (10)

comment:1 Changed 6 weeks ago by teor

Cc: nickm added
Keywords: network-team-roadmap-november 042-backport added
Parent ID: #29211
Points: 0.5
Sponsor: Sponsor31-must

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

comment:2 Changed 6 weeks ago by teor

Owner: set to teor
Status: newassigned

comment:3 Changed 5 weeks ago by teor

Owner: changed from teor to nickm

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?

comment:4 Changed 5 weeks ago by 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.

comment:5 Changed 5 weeks ago by teor

Owner: changed from nickm to 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.

comment:6 Changed 5 weeks ago by teor

The 0.4.3 ticket is #32404.

comment:7 in reply to:  4 Changed 5 weeks ago by 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.)

comment:8 Changed 5 weeks ago by teor

Actual Points: 0.2
Reviewer: nickm
Status: assignedneeds_review

See my PRs:

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

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

Last edited 5 weeks ago by teor (previous) (diff)

comment:9 Changed 5 weeks ago by nickm

Status: needs_reviewmerge_ready

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

comment:10 Changed 5 weeks ago by teor

Resolution: fixed
Status: merge_readyclosed

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.

Note: See TracTickets for help on using tickets.