[lm-sensors] PATCH: make lm_sensors init script return values LSB

[lm-sensors] PATCH: make lm_sensors init script return values LSB

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

Hi All,

I've received the attached patch through Fedora's bugzilla. It fixes the return 
values of the various error exit cases to match the LSB spec:
http://refspecs.linux-foundation.org/LSB_3.2.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html

It looks sane to me, if there are no objections I'll commit it to svn.

Thanks & Regards,

Hans


[-- Attachment #2: lm_sensors-lsb-retcode.diff --]
[-- Type: text/x-patch, Size: 827 bytes --]

--- lm_sensors.orig	2008-02-07 11:37:22.000000000 -0500
+++ lm_sensors	2008-02-07 11:41:04.000000000 -0500
@@ -40,15 +40,15 @@
 
 	# Don't bother if /proc/sensors still doesn't exist, kernel doesn't have
 	# support for sensors.
-	[ -e /proc/sys/dev/sensors ] || exit 0
+	[ -e /proc/sys/dev/sensors ] || exit 6
 
 	# If sensors was not already running, unload the module...
 	[ -e /var/lock/subsys/lm_sensors ] || /sbin/modprobe -r i2c-proc >/dev/null 2>&1
 fi
 
 CONFIG=/etc/sysconfig/lm_sensors
-[ -r "$CONFIG" ] || exit 0
-grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 0
+[ -r "$CONFIG" ] || exit 6
+grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 6
 
 # Load config file
 . "$CONFIG"
@@ -147,7 +147,7 @@
 	;;
   *)
 	echo "Usage: $0 {start|stop|status|restart|reload|condrestart}"
-	exit 1
+	exit 3
 esac
 
 exit $RETVAL

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] PATCH: make lm_sensors init script return values

[Jean Delvare]

Hi Hans,

On Mon, 11 Feb 2008 13:04:10 +0100, Hans de Goede wrote:
> I've received the attached patch through Fedora's bugzilla. It fixes the return 
> values of the various error exit cases to match the LSB spec:
> http://refspecs.linux-foundation.org/LSB_3.2.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
> 
> It looks sane to me, if there are no objections I'll commit it to svn.

> --- lm_sensors.orig	2008-02-07 11:37:22.000000000 -0500
> +++ lm_sensors	2008-02-07 11:41:04.000000000 -0500
> @@ -40,15 +40,15 @@
>  
>  	# Don't bother if /proc/sensors still doesn't exist, kernel doesn't have
>  	# support for sensors.
> -	[ -e /proc/sys/dev/sensors ] || exit 0
> +	[ -e /proc/sys/dev/sensors ] || exit 6

Note that this doesn't work with a 2.6 kernel anyway, which makes me
wonder whether it's worth keeping these init files in the lm-sensors
tree. Obviously no distribution uses them as-is.

>  
>  	# If sensors was not already running, unload the module...
>  	[ -e /var/lock/subsys/lm_sensors ] || /sbin/modprobe -r i2c-proc >/dev/null 2>&1
>  fi
>  
>  CONFIG=/etc/sysconfig/lm_sensors
> -[ -r "$CONFIG" ] || exit 0
> -grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 0
> +[ -r "$CONFIG" ] || exit 6
> +grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 6

I am worried that this error check (and the one above) is done
independently of the command, i.e. also for command "status", while the
document you mentioned above states that the error codes are different
for this command, and "6" isn't valid there. So I think that the
configuration file check and loading should be moved to the specific
commands that need it (start and stop as far as I can see) before you
can return 6 on missing configuration file.

>  
>  # Load config file
>  . "$CONFIG"
> @@ -147,7 +147,7 @@
>  	;;
>    *)
>  	echo "Usage: $0 {start|stop|status|restart|reload|condrestart}"
> -	exit 1
> +	exit 3

None of the init scripts in openSuse does this. They all use "exit 1",
and that sounds reasonable to me. If the user runs "lm_sensors blah",
it will fail, not because it is an "unimplemented feature" (3) but
because the user typed a command that doesn't exist. So I wouldn't
change it, but if you really don't like 1, then 2 ("invalid or excess
argument(s)") would be a better choice (we have one openSuse script
that does this.)

>  esac
>  
>  exit $RETVAL


-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] PATCH: make lm_sensors init script return values

[Hans de Goede]

Jean Delvare wrote:
> Hi Hans,
> 

Hi Jean,

Thanks for the thorough review!

> On Mon, 11 Feb 2008 13:04:10 +0100, Hans de Goede wrote:
>> I've received the attached patch through Fedora's bugzilla. It fixes the return 
>> values of the various error exit cases to match the LSB spec:
>> http://refspecs.linux-foundation.org/LSB_3.2.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
>>
>> It looks sane to me, if there are no objections I'll commit it to svn.
> 
>> --- lm_sensors.orig	2008-02-07 11:37:22.000000000 -0500
>> +++ lm_sensors	2008-02-07 11:41:04.000000000 -0500
>> @@ -40,15 +40,15 @@
>>  
>>  	# Don't bother if /proc/sensors still doesn't exist, kernel doesn't have
>>  	# support for sensors.
>> -	[ -e /proc/sys/dev/sensors ] || exit 0
>> +	[ -e /proc/sys/dev/sensors ] || exit 6
> 
> Note that this doesn't work with a 2.6 kernel anyway, which makes me
> wonder whether it's worth keeping these init files in the lm-sensors
> tree. Obviously no distribution uses them as-is.
> 

It helps to read the whole script before jumping to conclusions like this, 
above this is:
if grep -q sysfs /proc/mounts; then
         WITHSYS=1
else
         WITHSYS=0
fi

if [ $WITHSYS = "0" ]; then

So this piece of code doesn't get executed on 2.6 kernels with sysfs enabled. 
And actually atleast one distro (Fedora) is using the scrip as is.

>>  
>>  	# If sensors was not already running, unload the module...
>>  	[ -e /var/lock/subsys/lm_sensors ] || /sbin/modprobe -r i2c-proc >/dev/null 2>&1
>>  fi
>>  
>>  CONFIG=/etc/sysconfig/lm_sensors
>> -[ -r "$CONFIG" ] || exit 0
>> -grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 0
>> +[ -r "$CONFIG" ] || exit 6
>> +grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 6
> 
> I am worried that this error check (and the one above) is done
> independently of the command, i.e. also for command "status", while the
> document you mentioned above states that the error codes are different
> for this command, and "6" isn't valid there. So I think that the
> configuration file check and loading should be moved to the specific
> commands that need it (start and stop as far as I can see) before you
> can return 6 on missing configuration file.
> 

Good point I'll do a new version of the patch where all these checks are put in 
a function and that function only gets called from the relevant commands.

>>  
>>  # Load config file
>>  . "$CONFIG"
>> @@ -147,7 +147,7 @@
>>  	;;
>>    *)
>>  	echo "Usage: $0 {start|stop|status|restart|reload|condrestart}"
>> -	exit 1
>> +	exit 3
> 
> None of the init scripts in openSuse does this. They all use "exit 1",
> and that sounds reasonable to me. If the user runs "lm_sensors blah",
> it will fail, not because it is an "unimplemented feature" (3) but
> because the user typed a command that doesn't exist. So I wouldn't
> change it, but if you really don't like 1, then 2 ("invalid or excess
> argument(s)") would be a better choice (we have one openSuse script
> that does this.)
> 

Okay, I'll change things to keep exit 1 in this case.

Regards,

Hans


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] PATCH: make lm_sensors init script return values

[Jean Delvare]

Hi Hans,

On Wed, 13 Feb 2008 13:07:33 +0100, Hans de Goede wrote:
> Jean Delvare wrote:
> > On Mon, 11 Feb 2008 13:04:10 +0100, Hans de Goede wrote:
> >> I've received the attached patch through Fedora's bugzilla. It fixes the return 
> >> values of the various error exit cases to match the LSB spec:
> >> http://refspecs.linux-foundation.org/LSB_3.2.0/LSB-Core-generic/LSB-Core-generic/iniscrptact.html
> >>
> >> It looks sane to me, if there are no objections I'll commit it to svn.
> > 
> >> --- lm_sensors.orig	2008-02-07 11:37:22.000000000 -0500
> >> +++ lm_sensors	2008-02-07 11:41:04.000000000 -0500
> >> @@ -40,15 +40,15 @@
> >>  
> >>  	# Don't bother if /proc/sensors still doesn't exist, kernel doesn't have
> >>  	# support for sensors.
> >> -	[ -e /proc/sys/dev/sensors ] || exit 0
> >> +	[ -e /proc/sys/dev/sensors ] || exit 6
> > 
> > Note that this doesn't work with a 2.6 kernel anyway, which makes me
> > wonder whether it's worth keeping these init files in the lm-sensors
> > tree. Obviously no distribution uses them as-is.
> > 
> 
> It helps to read the whole script before jumping to conclusions like this, 
> above this is:
> if grep -q sysfs /proc/mounts; then
>          WITHSYS=1
> else
>          WITHSYS=0
> fi
> 
> if [ $WITHSYS = "0" ]; then
> 
> So this piece of code doesn't get executed on 2.6 kernels with sysfs enabled. 

Oops, you're right, I totally missed that, probably because
lm_sensors.init.suse doesn't have this conditional.

> And actually atleast one distro (Fedora) is using the scrip as is.

The script we ship in openSuse is based on lm_sensors.init.suse, but
it's heavily patched. In fact the patch is bigger than the script
itself. My impression is that lm_sensors.init.suse has not been updated
in parallel with lm_sensors.init and is totally lagging behind by now.
So I'd rather delete it, and let openSuse base their patch on
lm_sensors.init (which is certainly a saner base) or even just maintain
their own init script.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] PATCH: make lm_sensors init script return values

[Hans de Goede]

[-- Attachment #1: Type: text/plain, Size: 1952 bytes --]

Jean Delvare wrote:
> Hi Hans,
> 
>>  
>>  	# If sensors was not already running, unload the module...
>>  	[ -e /var/lock/subsys/lm_sensors ] || /sbin/modprobe -r i2c-proc >/dev/null 2>&1
>>  fi
>>  
>>  CONFIG=/etc/sysconfig/lm_sensors
>> -[ -r "$CONFIG" ] || exit 0
>> -grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 0
>> +[ -r "$CONFIG" ] || exit 6
>> +grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 6
> 
> I am worried that this error check (and the one above) is done
> independently of the command, i.e. also for command "status", while the
> document you mentioned above states that the error codes are different
> for this command, and "6" isn't valid there. So I think that the
> configuration file check and loading should be moved to the specific
> commands that need it (start and stop as far as I can see) before you
> can return 6 on missing configuration file.
> 

Fixed, new version attached.

>>  
>>  # Load config file
>>  . "$CONFIG"
>> @@ -147,7 +147,7 @@
>>  	;;
>>    *)
>>  	echo "Usage: $0 {start|stop|status|restart|reload|condrestart}"
>> -	exit 1
>> +	exit 3
> 
> None of the init scripts in openSuse does this. They all use "exit 1",
> and that sounds reasonable to me. If the user runs "lm_sensors blah",
> it will fail, not because it is an "unimplemented feature" (3) but
> because the user typed a command that doesn't exist. So I wouldn't
> change it, but if you really don't like 1, then 2 ("invalid or excess
> argument(s)") would be a better choice (we have one openSuse script
> that does this.)
> 

I've kept the exit 3, this patch was submitted to make the initscripts work 
with some gui service configuration tools, these will never pass a non existing 
command, but might pass a not supported one, in which case exit 3 is correct. I 
don't think any non automated tools will care, and for the automated ones exit 
3 is the best return code I believe.

New proposed patch attached.

Thanks & Regards,

Hans

[-- Attachment #2: lm_sensors-3.0.1-lsb-retcodes.patch --]
[-- Type: text/plain, Size: 2751 bytes --]

--- lm_sensors-3.0.1/prog/init/lm_sensors.init	2008-02-26 14:37:51.000000000 +0100
+++ lm_sensors-3.0.1.new/prog/init/lm_sensors.init	2008-02-26 14:25:13.000000000 +0100
@@ -28,31 +28,6 @@
 # in order as normal shell variables with the special names:
 #    MODULE_1, MODULE_2, MODULE_3, etc.
 
-if grep -q sysfs /proc/mounts; then
-	WITHSYS=1
-else
-	WITHSYS=0
-fi
-
-if [ $WITHSYS == "0" ]; then
-	# If sensors isn't supported by the kernel, try loading the module...
-	[ -e /proc/sys/dev/sensors ] || /sbin/modprobe i2c-proc >/dev/null 2>&1
-
-	# Don't bother if /proc/sensors still doesn't exist, kernel doesn't have
-	# support for sensors.
-	[ -e /proc/sys/dev/sensors ] || exit 0
-
-	# If sensors was not already running, unload the module...
-	[ -e /var/lock/subsys/lm_sensors ] || /sbin/modprobe -r i2c-proc >/dev/null 2>&1
-fi
-
-CONFIG=/etc/sysconfig/lm_sensors
-[ -r "$CONFIG" ] || exit 0
-grep '^MODULE_' $CONFIG >/dev/null 2>&1 || exit 0
-
-# Load config file
-. "$CONFIG"
-
 PSENSORS=/usr/local/bin/sensors
 
 if [ ! -x $PSENSORS ]; then
@@ -65,7 +40,47 @@
 RETVAL=0
 prog="lm_sensors"
 
+# This functions checks if sensor support is compiled into the kernel, if
+# sensors are configured, and loads the config file
+check_sensors() {
+	if grep -q sysfs /proc/mounts; then
+		WITHSYS=1
+	else
+		WITHSYS=0
+	fi
+
+	if [ $WITHSYS == "0" ]; then
+		# If sensors isn't supported by the kernel, try loading the module...
+		[ -e /proc/sys/dev/sensors ] || /sbin/modprobe i2c-proc >/dev/null 2>&1
+
+		# Don't bother if /proc/sensors still doesn't exist, kernel doesn't have
+		# support for sensors.
+		if ! [ -e /proc/sys/dev/sensors ]; then
+			echo -n "Starting $prog: kernel does not have sensors support"
+			echo_failure
+			echo
+			exit 5
+		fi
+
+		# If sensors was not already running, unload the module...
+		[ -e /var/lock/subsys/lm_sensors ] || /sbin/modprobe -r i2c-proc >/dev/null 2>&1
+	fi
+
+	CONFIG=/etc/sysconfig/lm_sensors
+	if ! [ -r "$CONFIG" ] || ! grep '^MODULE_' $CONFIG >/dev/null 2>&1; then
+		echo -n "Starting $prog: not configured, run sensors-detect"
+		echo_warning
+		echo
+		exit 6
+	fi
+
+	# Load config file
+	. "$CONFIG"
+}
+
 start() {
+	check_sensors
+
 	echo -n "Starting $prog: loading module "
 
 	modules=`grep \^MODULE_ $CONFIG | wc -l | tr -d ' '`
@@ -89,6 +104,8 @@
 }
 
 stop() {
+	check_sensors
+
 	echo -n "Stopping $prog: "
 
 	modules=`grep \^MODULE_ $CONFIG | wc -l | tr -d ' '`
@@ -116,12 +133,14 @@
 dostatus() {
 	$PSENSORS
 	RETVAL=$?
+	if [ $RETVAL -ne 0 ]; then
+		RETVAL=3
+	fi
 }
 
 restart() {
 	stop
 	start
-	RETVAL=$?
 }
 
 condrestart() {
@@ -147,7 +166,7 @@
 	;;
   *)
 	echo "Usage: $0 {start|stop|status|restart|reload|condrestart}"
-	exit 1
+	exit 3
 esac
 
 exit $RETVAL

[-- Attachment #3: Type: text/plain, Size: 153 bytes --]

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors