Opened 5 weeks ago

Closed 3 weeks ago

#31999 closed defect (implemented)

Default log file is handled inconsistently

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords:
Cc: nickm, teor Actual Points: 1.5
Parent ID: #31241 Points:
Reviewer: teor Sponsor: Sponsor31-can

Description

Our default logging setting is either "Log notice stdout" or "Log warn stdout" or "", depending on the command-line options. Right now, this is set in options_validate, which should not be setting anything at all. It's also set at the wrong time: defaults should be set in get_options_defaults(), so that they have the right semantics when you try to replace them or extend them.

I'll be fixing this as part of the parent ticket, but I'm making a separate entry here so we can refer to this bug in particular.

Child Tickets

Change History (8)

comment:1 Changed 5 weeks ago by nickm

This logic is less simple than I had thought a moment ago.

If we are using one of the functions that manipulates the logs (like --verify-config or --dump-config), we will be in --quiet/--hush mode, but we want to validate/dump the logs as they would be if we were running without --quiet/--hush.

comment:2 Changed 4 weeks ago by nickm

I have a better approach here in a branch called from_setconf_removal with a PR in https://github.com/torproject/tor/pull/1425 . I'm going to see what coveralls says, then write some tests and a better explanation of the change.

comment:3 Changed 4 weeks ago by nickm

Reviewer: teor
Status: newneeds_review

This branch isn't done yet, but I'd like your feedback on the general approach and on testing.

comment:4 Changed 4 weeks ago by teor

Status: needs_reviewneeds_revision

Seems fine to me.

I was wondering how we should test this as I was reading it:

  • can we test the individual functions like we used to test options_validate()?
  • should we try to test using our command-line harness?

It would also be nice to remove unused variables in this ticket:

/* 29211 TODO: Remove this from the API. */

comment:5 Changed 4 weeks ago by nickm

Our command-line harness can tell what we accept and don't accept, but can't tell whether we actually obey the settings properly. I think this is going to need a unit test. Our existing test_options.c code is pretty wonky, but I'll try to come up with something reasonable.

It would also be nice to remove unused variables in this ticket:

/* 29211 TODO: Remove this from the API. */

I'd actually like to do this after the next branch I have in mind here, where I plan to make the "default_options" variable unused. After that, I'm hoping I can use coccinelle to remove both variables entirely.

comment:6 Changed 3 weeks ago by nickm

Actual Points: 1.5
Status: needs_revisionneeds_review

I've added some tests, to make sure that the modified logic gets tests, as well as some of the old code that I didn't test.

comment:7 Changed 3 weeks ago by teor

Status: needs_reviewmerge_ready

Looks good, I'll merge it later today, if no-one else gets to it first :-)

comment:8 Changed 3 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Just merged to master.

Note: See TracTickets for help on using tickets.