FS#10536 - [initscripts] internal: save a pid from killall5

Attached to Project: Arch Linux
Opened by Anton Fiuman (lexiw) - Friday, 30 May 2008, 02:16 GMT
Last edited by Eric Belanger (Snowman) - Thursday, 02 June 2011, 22:59 GMT
Task Type Feature Request
Category Arch Projects
Status Closed
Assigned To Jan de Groot (JGC)
Aaron Griffin (phrakture)
Thomas Bächler (brain0)
Tom Gundersen (tomegun)
Architecture All
Severity Very Low
Priority Normal
Reported Version 2007.08-2
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 4
Private No

Details

Description:
A little patch to killall5 from the sysvinit package.
This patch allow users to set a pid that will be omitted from the kill routine.
Others distros have a similar patched killall5 but following another path. The way they implemented it need a command line parameter to be passed (killall5 -o omitpids). In this way a user can pass a list of pids to be omitted.
The way i did it is a bit different because my needs were a little different: i still need my process to not be killed but i don't want to patch/modify /etc/rc.shutdown. This is the last step needed to make an initscripts-splash that doesn't conflict/replace with the official initscritps package. During shutdown splashy (or any other userspace splash system) get killed to early, this patch avoids this.

How it works:
export KILLALL5_OMITPID="splashy"
/sbin/killall5 -15
/sbin/killall5 -9

The patch is very very small, i tried to make it as simple as possible to avoid bugs in a critical package.
If KILLALL5_OMITPID isn't set or something goes wrong i return a safe pid (return getpid()).
The patch is limited because i don't want to risk a bug, only one program is accepted.

The "alternative" patch doesn't work with program names but directly with pids. I don't like it even if it's shorter because it's too error prone (a wrong variable set and atoi fails, maybe a rewrite using strtol...).

How it works:
export KILLALL5_OMITPID="`pidof splashy`"
/sbin/killall5 -15
/sbin/killall5 -9

I'm using the first patch now and dismissed the alternative one.
This task depends upon

Closed by  Eric Belanger (Snowman)
Thursday, 02 June 2011, 22:59 GMT
Reason for closing:  Implemented
Additional comments about closing:  initscripts-2011.06.2-1
Comment by Aaron Griffin (phrakture) - Friday, 30 May 2008, 02:26 GMT
Would you mind attaching the patch?

My only thought is:
We can implement this two ways.
* Use the patch
* Replace rc.shutdown's killall5 call with a loop over all PIDs, doing the same sort of skip

Opinions?
Comment by Anton Fiuman (lexiw) - Friday, 30 May 2008, 02:34 GMT
I did attached the patches, don't know what happened :)
Comment by Jan de Groot (JGC) - Friday, 30 May 2008, 09:03 GMT
I'd like to see the patch implemented instead of simulating the functionality of killall5 in shell scripting. Shell scripting usually is much slower than a native C binary and produces extra overhead.
Comment by Aaron Griffin (phrakture) - Friday, 30 May 2008, 14:57 GMT
Jan, if you're cool with patching this, I'll definitely trust you on this one.

Lets go for it. Do we want to use one of the above patches, or Ubuntu's? The curious thing is that the above patches only let you omit _one_ PID instead of a list (i.e. KILLALL5_OMITPID="1 17 343 342345")
Comment by Anton Fiuman (lexiw) - Friday, 30 May 2008, 15:54 GMT
Yes, it was intended to be as small as possible :)
If you want to modify rc.shutdown too we can avoid the getenv call and add the "-o list_of_pids" options. A program would write it's pid to a file if he want it to be omitted and a script will load it and pass the arguments to killall5.

Here's how it works in debian:
mkdir -p /lib/init/rw/sendsigs.omit.d
pidof $NAME >> /lib/init/rw/sendsigs.omit.d/splashy
Comment by Jan de Groot (JGC) - Friday, 30 May 2008, 16:55 GMT
I guess the -o option needs the ubuntu/debian patch also? Can you apply it here, preferable with a patch to the manpage aswel to describe the new option? (I guess debian should have documented it by now).
Comment by Anton Fiuman (lexiw) - Friday, 30 May 2008, 22:45 GMT
I'll take care of porting the patch, after that you can decide what to do. The debian "omit pid system" recently changed, i don't know the rationale behind it (why a file per pid rather than a single file for every pid?). We should investigate further about this change. I'll try to take care about that too.
Searching in google it seems that this problem affects just (userspace) splash systems and (userspace) filesystems. http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=476698 and https://bugs.launchpad.net/ubuntu/+source/sysvinit/+bug/87763, maybe the latter doesn't affects ArchLinux at all, correct me if I'm wrong.
Anyway, in the future we'll probably need to omit something else so it should be better to plan accordingly.

To sum it up:
My patch is smaller, safe but limited to one pid.
The Debian patch is more versatile and it should be safe too (it's in Debian and Ubuntu after all) although it's bigger.

Pros:
My patch doesn't require to modify rc.shutdown.
Debian patch is the right way to do things (-o omitpid is an options of pidof too...).

Cons:
My patch is dirty because i needed a workaround to NOT modify rc.shutdown.
Debian patch requires to modify rc.shutdown: load pids from a file (or files) and build the command line options to be passed to killall5.
Comment by Anton Fiuman (lexiw) - Saturday, 31 May 2008, 21:03 GMT
Ok, i ported the patch from Debian/Ubuntu. I slightly modified it and added the new usage print (only the man page was patched).
I have ready a patch for rc.shutdown, i preferred the "file per script" way to make it possible for scripts that need to modify the list of pids at run time without race conditions.

The format is: /var/run/shutdown.omit.d/packagename

How it works:
mkdir -p /var/run/shutdown.omit.d/
pidof name > /var/run/shutdown.omit.d/name

or:

mkdir -p /var/run/shutdown.omit.d/
rm -f /var/run/shutdown.omit.d/name
ln -s /var/run/name.pid /var/run/shutdown.omit.d/name

I've attached a patch for the sysvinit PKGBUILD.
Comment by kujub (kujub) - Sunday, 06 September 2009, 08:57 GMT
This would be very useful for initscripts-extras-fbsplash too. Status of this ?
Comment by Thomas Bächler (brain0) - Monday, 21 September 2009, 22:36 GMT
Hmm, no idea, what's the opinion on this patch? What's happening upstream in sysvinit, is there even active upstream?
Comment by Dan Griffiths (Ghost1227) - Monday, 15 February 2010, 22:49 GMT
+1 from me...
Comment by Benjamin Richter (Waldteufel) - Saturday, 06 March 2010, 16:05 GMT
It seems that sysvinit is still developed. At least there are commits that are only about week old at http://svn.savannah.gnu.org/viewvc/sysvinit/trunk/?root=sysvinit

And as far as i can see, there already is an -o option (for “omit”) in the svn version of killall5 http://svn.savannah.gnu.org/viewvc/sysvinit/trunk/src/killall5.c?root=sysvinit&view=markup (scroll down to the bottom)
Comment by kujub (kujub) - Sunday, 02 May 2010, 17:31 GMT
sysvinit 2.88 seems to be out on the new hp http://savannah.nongnu.org/projects/sysvinit
Comment by kujub (kujub) - Thursday, 26 August 2010, 14:59 GMT
Here comes the PKGBUILD for upgrading sysvinit:
Edit: actually attached files
Comment by kujub (kujub) - Thursday, 26 August 2010, 19:09 GMT
... and now the patch for initscripts which should solve this finally (:
edit: corrected patch
edit: corrected again :-/ (can not delete - please use 3rd one)
Comment by Eric Belanger (Snowman) - Friday, 10 September 2010, 17:11 GMT
FYI: sysvinit-2.88-1 is now in testing
Comment by kujub (kujub) - Friday, 10 September 2010, 18:13 GMT
Thank you. Here is the updated patch for initscripts git:
Comment by Gerardo Exequiel Pozzi (djgera) - Tuesday, 01 February 2011, 04:46 GMT
I tested the last patch some days ago and works fine. I used it for testing archiso with uniofs-fuse that needs a daemon running all time.

Loading...