Opened 4 months ago

Closed 4 months ago

#32339 closed enhancement (fixed)

Use a table to drive options_check_transition and warn_about_relative_paths

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-october
Cc: Actual Points: .1
Parent ID: #29211 Points: 1
Reviewer: teor Sponsor: Sponsor31-can

Description

We regularly forget to add files to both of these functions.
We should make them part of the standard options table.

Child Tickets

TicketTypeStatusOwnerSummary
#32338defectclosedteorWarn about more relative file paths when validating options

Change History (9)

comment:1 Changed 4 months ago by nickm

See #32344 for part of this.

comment:2 Changed 4 months ago by nickm

Actual Points: .1
Reviewer: teor
Status: newneeds_review

I have the relative-path part of this in branch ticket32339_relative with PR at https://github.com/torproject/tor/pull/1487

comment:3 Changed 4 months ago by teor

Status: needs_reviewneeds_revision

Looks fine, but should we also be checking file Logs here?

comment:4 Changed 4 months ago by nickm

We should, but that's harder and I don't know a good way to do it. Any ideas?

comment:5 Changed 4 months ago by teor

We should be able to loop through Logs like we do HiddenServiceDir?
Maybe we'll need to do that after we've parsed Logs into log objects.
I'll see if I can write some code for it.

comment:6 Changed 4 months ago by nickm

Status: needs_revisionneeds_review

One challenge with looping over logs is that we only want to look at the filenames. We'd also want to look at filenames inside FooPort options

My eventual hope with Logs and Ports is that we can reuse the control-command parsing code in control_cmd.c to break them into their own options objects, and handle those with the regular configuration processing code.

Do you think that, for now, we might be able to take this patch as a straightforward refactoring, and add additional checks in a separate branch? For me it is a blocker for #30866, and I'd like to get that straightened out.

comment:7 Changed 4 months ago by nickm

(Rationale for proceeding: I believe that this branch is a strict improvement over Tor's current behavior, doesn't introduce technical debt, and unblocks #30866. If you don't agree, we shouldn't proceed.)

comment:8 Changed 4 months ago by teor

Status: needs_reviewmerge_ready

Yes, I think we should go ahead, and make further changes in other tickets.

I'd like to merge #32213, #32339, and #32344 in that order, to minimise conflicts (if any). I'm just waiting for CI to pass on #32213.

comment:9 Changed 4 months ago by teor

Keywords: network-team-roadmap-october added; network-team-roadmap-? removed
Resolution: fixed
Status: merge_readyclosed

Merged #32213, #32339, and #32344.

Resolved conflicts between #32339 and #32344 by taking the V_IMMUTABLE and FILENAME changes on PidFile.

Opened #32373 for follow up.

Note: See TracTickets for help on using tickets.