spanezz / nodm

Automatic display manager
GNU General Public License v2.0
141 stars 21 forks source link

Fixes: When systemd tries to kill nodm, nodm restarts itself instead #5 #10

Open slashblog opened 6 years ago

slashblog commented 6 years ago

The issue is that shutdown_quit_notification blocks signals. In nodm_display_manager_wait functions there is waitpid call. This returns -1 if signal was received before session (dm->session.pid) was closed by systemctl. This is the happy scenario.

However, if session gets closed before signal was sent to the process. The waitpid returns dm->session.pid and the call to shutdown_quit_notification blocks SIGTERM. Due to this, catch_signals is never called and nodm proceeds to restarting display manager.

I have removed function shutdown_quit_notification altogether, as I don't see any need for it, since IMO there is no need to block these signals.

sunweaver commented 6 years ago

@slashblog: Thanks for working on issue #5.

However, your pull request is formalistically unclean. I'd be happy, if you are willing to...

To achieve this, you probably need to use git on the command line or some Git GUI. The Github WebUI stuff will be insufficient for handling this, I guess.

Are you willing to perform the requested changes? If not, please let me know. For an updated branch, please do a git push --force :. This will automatically update this PR.

THANKS!

avindra commented 6 years ago

@sunweaver Github allows you to:

without having the original author (@slashblog) getting involved.

Lauszus commented 6 years ago

Just tested this on a Raspberry Pi running Raspbian Jessie and this PR solves the issue with nodm restarting itself when it is shutting down.

j00ch commented 6 years ago

I tested this commit on my Manjaro system, running Openbox and systemd. No fix here sadly, still get the PID error and then nodm restarts itself.

darkspace commented 6 years ago

This fixed the restart issue on my Intel Compute Stick running Ubuntu 16.04. Thanks @slashblog!

dustydecapod commented 6 years ago

For the love of god, someone merge this already.

spanezz commented 6 years ago

I am not active in nodm maintenance anymore. @sunweaver, can you look into this, or else add other people as maintainers?

sunweaver commented 6 years ago

Hi,

On Mi 21 Feb 2018 13:22:01 CET, Enrico Zini wrote:

I am not active in nodm maintenance anymore. @sunweaver, can you
look into this, or else add other people as maintainers?

@spanezz: I need to set up a test VM and then, yes. I will take a
look. Low prio though. In any way, the commits are unclean and need
rework.

Mike --

DAS-NETZWERKTEAM mike gabriel, herweg 7, 24357 fleckeby mobile: +49 (1520) 1976 148 landline: +49 (4354) 8390 139

GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 mail: mike.gabriel@das-netzwerkteam.de, http://das-netzwerkteam.de

kaey commented 6 years ago

Fix works for me