[Pm-utils] POSIXification of pm-utils

Stefan Seyfried seife at suse.de
Wed Jan 30 03:32:21 PST 2008


On Thu, Jan 17, 2008 at 10:10:23PM -0600, Victor Lowther wrote:

> The attached patch (against cvs head) makes all the scripting in
> pm-utils POSIX compliant.  This patch has been tested on a basic

I still think this is unnecessary (if not stupid).

> install of Debian sid, with the Debian native pm-utils overwritten by
> this one and dash installed as /bin/sh.  Debian is able to
> hibernate/resume normally.

> --- pm-utils/pm/functions	2007-12-29 05:58:18.000000000 -0600
> +++ working/pm/functions	2008-01-14 16:44:38.000000000 -0600

> +# first, source our config files
> +for cfg in /etc/pm/config.d/*[!~] ; do
> +	[ -f "$cfg" ] || continue
> +	set -a
> +	. "${cfg}"

If a user has created his own hooks, using "bashisms", knowing that
pm-utils is using bash, this will break. How are you handling that?

> +# spin waiting for the lock with optional timeout.  
> +# return once we have it, or the timeout has expired
> +spin_lock()
> +{
> +	# $1 = directory to use as the lock directory
> +	# $2 = optional timeout
> +	local elapsed=0
> +	while ! try_lock $1; do
> +		sleep 1;
> +		[ "x$2" != "x" ] && [ $(( $elapsed == $2 )) -ne 0 ] && return 1
>  	done
>  }

This only works if timeout is 0.

> -source_configs
> +# release the lock
> +release_lock()
> +{
> +	# $1 = directory used as the lock directory
> +	# make sure it is ours first.
> +	rm -rf "$1"
> +	return $?
> +}

Make sure that nobody can trick your code into calling this with "/" as
parameter...

>  pm_main()
>  {
> -	if [ -n "$PM_LOGFILE" ]; then
> +	if [ "$PM_LOGFILE" ]; then
>  		exec > "$PM_LOGFILE" 2>&1
>  	fi
>  	take_suspend_lock || exit 1
>  
> -	rm -f "$INHIBIT"
> -
> -	run_hooks "$1"
> +	# make sure that our locks are unlocked no matter how the script exits
> +	trap remove_suspend_lock 0
>  
> -	if [ ! -e "$INHIBIT" -a "$(type -t "do_$1")" == "function" ]; then
> -		sync ; sync ; sync
> -		"do_$1"
> -	fi
> -
> -	run_hooks "$2" reverse
> +	rm -f "$INHIBIT"
>  
> -	remove_suspend_lock 200
> +	run_hooks sleep "$1" reverse "$2"
>  
>  	return 0
>  }

i somehow miss the actuall suspend invocation here. Did you also outsource
that into a hook? Why?

>  _rmmod() {
>  	if modprobe -r $1; then
> -		echo "export RESUME_MODULES=\"$1 \$RESUME_MODULES\"" \
> -						>> /var/run/pm-suspend
> +		touch "/var/run/pm-suspend/module:$1"
>  		return 0

You lose the information about the unload order, which might be useful.

>  modunload()
>  {
>  	local MOD D C USED MODS I
> -	local UNL=$1 RET=1
> -	# the kernel only knows underscores in module names, no dashes
> -	UNL=${UNL//-/_}
> -	# RET is the return code.
> -	# If at least one module was unloaded, return 0.
> -	# if the module was not loaded, also return 0 since this is no error.
> -	# if no module was unloaded successfully, return 1

I'm not sure that those comments are useless, since the code is quite
convulted and unreadable, but i don't need them ;-)

> +# reload all the modules in no particular order.

Not necessarily a good idea.

>  stopservice()
>  {
> -	service "$1" status 2>/dev/null | grep -c -q running
> -	if [ "$?" == "0" ]; then
> -		N=${1//[-.]/_}
> -		echo "export ${N}_SERVICE_ACTIVATE=yes" >> /var/run/pm-suspend
> +	if service "$1" status 2>/dev/null | grep -c -q running; then
> +		touch "/var/run/pm-suspend/service:$1"
>  		service "$1" stop
>  	fi
>  }

One nice thing about /var/run/pm-suspend was, that you could simply let the
bugreporter attach it to a bugreport and see what was going on.

> --- pm-utils/pm/hooks/95led	2007-03-06 11:56:11.000000000 -0600
> +++ working/pm/hooks/95led	2008-01-17 20:54:42.000000000 -0600
> @@ -1,6 +1,6 @@
> -#!/bin/bash
> +#!/bin/sh
>  
> -[ -f /proc/acpi/ibm/led ] || exit 0
> +[ -f /proc/acpi/ibm/led ] || exit 1

This is not an error. Most machines on this planet do not have /proc/acpi/ibm.

> diff -urNX diffignore pm-utils/pm/hooks/zzz working/pm/hooks/zzz
> --- pm-utils/pm/hooks/zzz	1969-12-31 18:00:00.000000000 -0600
> +++ working/pm/hooks/zzz	2008-01-17 20:54:42.000000000 -0600
> @@ -0,0 +1,10 @@
> +#!/bin/sh
> +. /usr/lib/pm-utils/functions
> +[ -e "$INHIBIT" ] && return 1
> +sync;sync;sync;

Cargo cult programming. Either one sync is enough, or we need to fix the
kernel. The kernel does sync later, anyway.

> +case $1 in
> +	suspend|hibernate|suspend_hybrid) do_$1 ;;
> +	resume|thaw) exit 0 ;;
> +	*) exit 1 ;;
> +esac
> +exit $?

This whole hook ist stupid, even though it make patching in a different
suspend method easier.

> --- pm-utils/pm/Makefile.am	2007-03-06 13:08:05.000000000 -0600
> +++ working/pm/Makefile.am	2008-01-17 21:42:17.000000000 -0600
> @@ -4,7 +4,7 @@
>  
>  extra_SCRIPTS =			\
>  	functions		\
> -	defaults
> +	defaults		

WTF?

> --- pm-utils/README	2006-10-30 12:17:03.000000000 -0600
> +++ working/README	2008-01-12 18:43:53.000000000 -0600
> @@ -26,7 +26,7 @@
>  
>  How do "hooks" work?
>  
> -* You put a file in /etc/pm/hooks, which is executable.  When suspend or
> +* You put an executable file in /etc/pm/sleep.d.  When suspend or
>    hibernate is called, several things happen:
>    
>    1) a new virtual terminal is alloced and switched to
> @@ -34,9 +34,9 @@
>       modified by end-users.
>    3) /etc/pm/config.d/* are evaluated in C sort order.  These files can be
>       provided by individual packages outside of pm-utils.  If a global config
> -     variable is set, the value set to will be appended to the previous value.
> +     variable is set, the value set to will overwrite the previous value.
>       If any other variable is set, it will be ignored.
> -  4) each of /etc/pm/hooks/* are executed in C sort order.  The first command
> +  4) each of /etc/pm/sleep.d/* are executed in C sort order.  The first command
>       line argument is "suspend" or "hibernate".  These files may source
>       configuration files from /etc/pm/config.d/ on their own in order to pick
>       up variables set there that aren't part of the global list.  Note that
> @@ -45,7 +45,7 @@
>       will clobber any such variables.
>    5) the system suspends or hibernates.
>    6) some event happens to wake the machine up
> -  7) each of /etc/pm/hooks/* are executed in reverse C sort order.  The first
> +  7) each of /etc/pm/sleep.d/* are executed in reverse C sort order.  The first
>       command line argument is "resume" or "thaw".
>    8) the system switches back to the original virtual terminal from step 1.

Thanks for those fixes.
But you should spell out clear and loud that the hooks now need to be able
to run in a crippled shell.

-- 
Stefan Seyfried
R&D Team Mobile Devices            |              "Any ideas, John?"
SUSE LINUX Products GmbH, Nürnberg | "Well, surrounding them's out." 

This footer brought to you by insane German lawmakers:
SUSE Linux Products GmbH, GF: Markus Rex, HRB 16746 (AG Nürnberg)


More information about the Pm-utils mailing list