vmatare / thinkfan

The minimalist fan control program
GNU General Public License v3.0
552 stars 62 forks source link

Use `$XDG_RUNTIME_DIR` #164

Closed savchenko closed 2 years ago

savchenko commented 2 years ago

thinkfan seems to invariably write its PID file in /run. Please query $XDG_RUNTIME_DIR and try to use it before falling-back to the default.

koutheir commented 2 years ago

One of the issues here is that setting XDG_RUNTIME_DIR to something different would allow multiple instances of thinkfan to run, which is exactly what the PID file is trying to forbid.

Furthermore, the PID file path is actually selected at configuration time, making it a compile-time constant. It is embedded in the thinkfan binary.

Using XDG_RUNTIME_DIR for the PID file requires ensuring that all instances of thinkfan run with the exact same value for that environment variable, in order to ensure that at most one instance is running all the time. The paranoid in me thinks it's a bad assumption. Verifying this assumption will require inter-process communications through some other means, which, if implemented, would make the PID file way less useful in the first place.

savchenko commented 2 years ago

Valid concerns. Quick & dirty:

  1. systemctl (--user) status ...
  2. ps ax | grep ...
  3. if $XDG_RUNTIME_DIR/foo.pid else /run/foo.pid ...

Not a big deal in my case, re-defined here: https://github.com/vmatare/thinkfan/blob/ab466ddd0ebf62d3fa5db1f6d1cc25ec1df1e9d4/CMakeLists.txt#L24 ...but might come as a surprise for users that don't have write access to /run

koutheir commented 2 years ago
  1. systemctl relies on a fixed PID file to control and report on the service process. So, as far as I know, it requires a fixed path of the PID file.
  2. Using ps ax and identifying processes by their names does not work because running another executable called thinkfan defeats it.
  3. if $XDG_RUNTIME_DIR/foo.pid else /run/foo.pid allows multiple instances to run if the value of XDG_RUNTIME_DIR is changed.

So all the above seem like workarounds, which still do not guarantee that, at most, one instance of thinkfan is running.

A middle ground solution to this might be to accept a new parameter that specifies where to put the PID file. This parameter would still default to a hard-coded value when unspecified. A person who specifies a custom value for that parameter would assume the responsibility of ensuring that ALL instances of thinkfan will be executed with that exact same parameter, too. I qualify this feature as nice to have.

vmatare commented 2 years ago

Hi @savchenko, have you experienced a particular case where /run was not the right choice? I'd be interested to learn what that was...

savchenko commented 2 years ago

@vmatare

$ mount | grep "\/run\ "
tmpfs on /run type tmpfs (rw,nosuid,nodev,noexec,relatime,size=3257992k,mode=755,inode64)
                                          ^^^^^^
                                           here
vmatare commented 2 years ago

I don't understand. What's wrong with noexec?

vmatare commented 2 years ago

Closing for lack of feedback and because the actual problem is unclear. @savchenko Feel free to reopen if you have new information.