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
Cc: | nickm added |
---|---|
Keywords: | network-team-roadmap-november 042-backport added |
Parent ID: | → #29211 |
Points: | → 0.5 |
Sponsor: | → Sponsor31-must |
comment:2 Changed 6 weeks ago by
Owner: | set to teor |
---|---|
Status: | new → assigned |
comment:3 Changed 5 weeks ago by
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 follow-up: 7 Changed 5 weeks ago by
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
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:7 Changed 5 weeks ago by
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
Actual Points: | → 0.2 |
---|---|
Reviewer: | → nickm |
Status: | assigned → needs_review |
comment:9 Changed 5 weeks ago by
Status: | needs_review → merge_ready |
---|
Looks correct. #32404 is already merged to master. Please merge as appropriate.
comment:10 Changed 5 weeks ago by
Resolution: | → fixed |
---|---|
Status: | merge_ready → closed |
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.
This is a bug on #30864, which was sponsor 31, so it is a must-have for Sponsor 31.