New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v239.1: PID file not removed when elogind is terminated #94

Closed
Yamakuzure opened this Issue Nov 9, 2018 · 7 comments

Comments

Projects
None yet
4 participants
@Yamakuzure
Copy link
Collaborator

Yamakuzure commented Nov 9, 2018

I got an e-mail from Stefan about elogind not removing its PID file if it is shutdown using signal 15 (SIGTERM).

As /run is on tmpfs nowadays, this is of no real concern if elogind crashes on reboot or if the whole machine comes down. However, not all systems are like that and some have not put /run into RAM.

On these systems elogind fails to start on next reboot if a stale PID file exists.

There are two reasons for this:

  1. The installation of the signal handlers is not correct. This is a follow-up on issue #90, which is not properly fixed by now.
  2. elogind does not check whether the PID in a found PID file actually points to an elogind instance, or is stale.

Stefan has already provided fixes for the 1st point, which I am currently testing.
The second point will be tackled thereafter.

@Yamakuzure Yamakuzure self-assigned this Nov 9, 2018

@Yamakuzure Yamakuzure added the bug label Nov 9, 2018

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator Author

Yamakuzure commented Nov 10, 2018

The second task, making elogind to actually check the PID file if one is found, is on my TODO list.

@daym

This comment has been minimized.

Copy link

daym commented Nov 10, 2018

Checking the PID in the PID file doesn't sound safe - what if it's coincidentially the same as it was in the previous boot (f.e. a deterministic boot process could easily make that happen every time).

Better would be not to use a pid file but something else. Doesn't elogind provide a dbus service? How come it's not enough to try to register the dbus service and if that doesn't work (because it's already there), fail?

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator Author

Yamakuzure commented Nov 11, 2018

Well, elogind checks the presence of a PID file, before it would create its own.

  • If the contained PID is its own, then everything is in order.
  • If the contained PID is different and belongs to an elogind[-daemon] process, then elogind is already running.
  • If the contained PID is different and belongs to something else. then it's a stale PID file that can be overwritten.

Unfortunately service managers like OpenRC need such a PID file to know where to send SIGTERM when shuting down the device.

@udeved

This comment has been minimized.

Copy link

udeved commented Nov 11, 2018

@Yamakuzure

Would openrc do if elogind service was supervised?
Runit doesn't need it afaik, it uses the supervisor pid.

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator Author

Yamakuzure commented Nov 12, 2018

@udeved : Not that I know of. The start-stop-daemon() command needs a PID file.

Well, I just noticed, that from the three points above the first two were already implemented, and the third was easy. See: 59e70b3

@markhindley

This comment has been minimized.

Copy link
Contributor

markhindley commented Dec 12, 2018

Sven,

I think the fix in 59e70b3 has an error. See `https://bugs.debian.org/916212.

The attached patch fixes it for me

Fix_stale_pidfile.patch.txt

Mark

@Yamakuzure

This comment has been minimized.

Copy link
Collaborator Author

Yamakuzure commented Dec 12, 2018

@markhindley : Yes, you are right.

@Yamakuzure Yamakuzure reopened this Dec 12, 2018

Yamakuzure added a commit that referenced this issue Dec 12, 2018

Prep v239.4: Fix elogind_is_already_running()
The function accidently returned the found pid, if it didn't belong
to a running process.

Bug: #94
Signed-Off-By: Sven Eden <sven.eden@prydeworx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment