Opened 14 months ago

Closed 6 weeks ago

#20119 closed enhancement (fixed)

Tor should exit if it fails to write its PidFile, under principle of least confusion. Also, maybe Tor should create the directory that the PidFile points to

Reported by: yurivict271 Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.2.8.7
Severity: Normal Keywords: easy, fs, tor-relay, usability, startup, review-group-23
Cc: Actual Points: 0
Parent ID: Points: .5
Reviewer: dgoulet Sponsor:

Description

Running with the option: --PidFile /var/run/tor/tor.pid
Directory /var/run/tor is missing. Tor starts without any warnings. It has to either create the directory, or complain that it can't create the pid file.

FreeBSD 10.3

Child Tickets

Change History (18)

comment:1 Changed 14 months ago by cypherpunks

Component: - Select a componentCore Tor/Tor

comment:2 Changed 12 months ago by nickm

Milestone: Tor: 0.3.0.x-final

comment:3 Changed 12 months ago by teor

This may be related to #20456, and if so, may also affect control_auth_cookie.

comment:4 Changed 10 months ago by dgoulet

Keywords: triage-out-030-201612 added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

Triaged out on December 2016 from 030 to 031.

comment:5 Changed 8 months ago by nickm

Points: .5

comment:6 Changed 8 months ago by nickm

Keywords: triaged-out-20170308 added
Milestone: Tor: 0.3.1.x-finalTor: unspecified

Deferring all 0.3.1 tickets with status == new, owner == nobody, sponsor == nobody, points > 0.5, and priority < high.

I'd still take patches for most of these -- there's just nobody currently lined up to work on them in this timeframe.

comment:7 Changed 6 months ago by arma

I do get a warning:

May 08 00:35:40.262 [warn] Unable to open "/tmp/path/to/unknown.pid" for writing: No such file or directory

it's not a very good one though, I agree.

(also, the warning might be showing up only on my stdout, since maybe it happens before Tor has set up its logs.)

comment:8 Changed 6 months ago by arma

On further thought, I think I agree with the original reporter: we need to either make the directory, or we need to complain _and exit_.

comment:9 Changed 5 months ago by nickm

Keywords: triage-out-030-201612 removed

comment:10 Changed 4 months ago by arma

This one is safe to treat orthogonally to the mess in #22731, since the write_pidfile() function in options_act() happens after the finish_daemon() call.

Shouldn't we be using check_private_dir() for the place where we're going to put our PidFile? And then we could pass it the CPD_CREATE flag?

Then if write_pidfile() fails, we should error out.

comment:11 Changed 4 months ago by arma

Summary: Fails to create the pid file when an enclosing directory is missingTor should exit if it fails to write its PidFile, under principle of least confusion. Also, maybe Tor should create the directory that the PidFile points to

comment:12 Changed 4 months ago by nickm

Keywords: easy fs tor-relay added

comment:13 Changed 4 months ago by nickm

Keywords: usability startup added; triaged-out-20170308 removed

comment:14 Changed 6 weeks ago by nickm

Actual Points: 0
Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Owner: set to nickm
Status: newaccepted
Type: defectenhancement

comment:15 Changed 6 weeks ago by nickm

Status: acceptedneeds_review

I've done the minimal version (no directory creation), as ticket20119 in my public repository.

(If we're using the principle of least surprise to justify ourselves here, let's not create the directory.)

comment:16 Changed 6 weeks ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:17 Changed 6 weeks ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

Agree with not creating the directory. Creating a hierarchy of dir is whole other ball game and could easily be another ticket to do such a feature.

lgtm;

comment:18 Changed 6 weeks ago by nickm

Resolution: fixed
Status: merge_readyclosed

merging!

Note: See TracTickets for help on using tickets.