[lm-sensors] PATCH: systemd integration

[lm-sensors] PATCH: systemd integration

[Hans de Goede]

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

Hi,

The attached patch:
1) uses systemd's systemctl to start / stop / enable the lm_sensors service on
systemd systems (such as Fedora 15)
2) recognizes the new /run dir for run time info (as used by systemd systems)
as a possible place where the udev db can live
3) adds a lm_sensor.service file to install under /lib/systemd/system

Please review, I'll push it to svn myself once acked.

Regards,

Hans


[-- Attachment #2: lm_sensors-3.3.0-systemd.patch --]
[-- Type: text/plain, Size: 1985 bytes --]

Index: prog/init/lm_sensors.service
===================================================================
--- prog/init/lm_sensors.service	(revision 0)
+++ prog/init/lm_sensors.service	(revision 0)
@@ -0,0 +1,14 @@
+[Unit]
+Description=lm_sensors for monitoring motherboard sensor values
+After=syslog.target
+
+[Service]
+EnvironmentFile=/etc/sysconfig/lm_sensors
+Type=oneshot
+RemainAfterExit=yes
+ExecStart=-/sbin/modprobe -qab $BUS_MODULES $HWMON_MODULES
+ExecStart=/usr/bin/sensors -s
+ExecStop=-/sbin/modprobe -qabr $BUS_MODULES $HWMON_MODULES
+
+[Install]
+WantedBy=multi-user.target
Index: prog/detect/sensors-detect
===================================================================
--- prog/detect/sensors-detect	(revision 5939)
+++ prog/detect/sensors-detect	(working copy)
@@ -2339,7 +2339,7 @@
 	if (!$use_udev) {
 		# Try some known default udev db locations, just in case
 		if (-e '/dev/.udev.tdb' || -e '/dev/.udev'
-		 || -e '/dev/.udevdb') {
+		 || -e '/dev/.udevdb' || -e '/run/udev') {
 			$use_udev = 1;
 			$dev_i2c = '/dev/i2c-';
 		}
@@ -6378,6 +6378,14 @@
 		}
 		close(SYSCONFIG);
 
+		if (-x "/bin/systemctl" &&
+		    -f "/lib/systemd/system/lm_sensors.service") {
+			system("/bin/systemctl", "enable", "lm_sensors.service");
+			system("/bin/systemctl", "start", "lm_sensors.service");
+			# All done, don't check for /etc/init.d/lm_sensors
+			return;
+		}
+
 		print "Copy prog/init/lm_sensors.init to /etc/init.d/lm_sensors\n".
 		      "for initialization at boot time.\n"
 			unless -f "/etc/init.d/lm_sensors";
@@ -6433,8 +6441,10 @@
 		exit -1;
 	}
 
-	if (-x "/sbin/service" && -f "/etc/init.d/lm_sensors" &&
-	    -f "/var/lock/subsys/lm_sensors") {
+	if (-x "/bin/systemctl" && -f "/lib/systemd/system/lm_sensors.service") {
+		system("/bin/systemctl", "stop", "lm_sensors.service");
+	} elsif (-x "/sbin/service" && -f "/etc/init.d/lm_sensors" &&
+		 -f "/var/lock/subsys/lm_sensors") {
 		system("/sbin/service", "lm_sensors", "stop");
 	}
 

[-- 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: systemd integration

[Jean Delvare]

Hi Hans,

On Sun, 24 Apr 2011 16:17:16 +0200, Hans de Goede wrote:
> The attached patch:
> 1) uses systemd's systemctl to start / stop / enable the lm_sensors service on
> systemd systems (such as Fedora 15)
> 2) recognizes the new /run dir for run time info (as used by systemd systems)
> as a possible place where the udev db can live
> 3) adds a lm_sensor.service file to install under /lib/systemd/system
> 
> Please review, I'll push it to svn myself once acked.

I don't know anything about systemd, but the patch looks good and I'm
all for better integration of upstream lm-sensors so that every
distribution doesn't have to do it all again.

Comments:

> Index: prog/init/lm_sensors.service
> =================================> --- prog/init/lm_sensors.service	(revision 0)
> +++ prog/init/lm_sensors.service	(revision 0)
> @@ -0,0 +1,14 @@
> +[Unit]
> +Description=lm_sensors for monitoring motherboard sensor values

lm-sensors is not (or shouldn't be) limited to motherboard sensors.
Sensors on CPU, memory modules and graphics cards are supported too, and
hopefully hard disk drive temperatures will be someday too.

> +After=syslog.target
> +
> +[Service]
> +EnvironmentFile=/etc/sysconfig/lm_sensors
> +Type=oneshot
> +RemainAfterExit=yes
> +ExecStart=-/sbin/modprobe -qab $BUS_MODULES $HWMON_MODULES
> +ExecStart=/usr/bin/sensors -s
> +ExecStop=-/sbin/modprobe -qabr $BUS_MODULES $HWMON_MODULES

Please keep in mind that both $BUS_MODULES and $HWMON_MODULES may be
empty, which would cause the above modprobe commands to return an
error. Still it is legitimate to start the service so that "sensors -s"
is executed.

> +
> +[Install]
> +WantedBy=multi-user.target
> Index: prog/detect/sensors-detect
> =================================> --- prog/detect/sensors-detect	(revision 5939)
> +++ prog/detect/sensors-detect	(working copy)
> @@ -2339,7 +2339,7 @@
>  	if (!$use_udev) {
>  		# Try some known default udev db locations, just in case
>  		if (-e '/dev/.udev.tdb' || -e '/dev/.udev'
> -		 || -e '/dev/.udevdb') {
> +		 || -e '/dev/.udevdb' || -e '/run/udev') {
>  			$use_udev = 1;
>  			$dev_i2c = '/dev/i2c-';
>  		}
> @@ -6378,6 +6378,14 @@
>  		}
>  		close(SYSCONFIG);
>  
> +		if (-x "/bin/systemctl" &&
> +		    -f "/lib/systemd/system/lm_sensors.service") {
> +			system("/bin/systemctl", "enable", "lm_sensors.service");
> +			system("/bin/systemctl", "start", "lm_sensors.service");
> +			# All done, don't check for /etc/init.d/lm_sensors
> +			return;
> +		}
> +
>  		print "Copy prog/init/lm_sensors.init to /etc/init.d/lm_sensors\n".
>  		      "for initialization at boot time.\n"
>  			unless -f "/etc/init.d/lm_sensors";

Don't you want to display a smiliar message for the systemd case? Users
installing manually will have to copy prog/init/lm_sensors.service
to /lib/systemd/system/lm_sensors.service for initialization at boot
time.

> @@ -6433,8 +6441,10 @@
>  		exit -1;
>  	}
>  
> -	if (-x "/sbin/service" && -f "/etc/init.d/lm_sensors" &&
> -	    -f "/var/lock/subsys/lm_sensors") {
> +	if (-x "/bin/systemctl" && -f "/lib/systemd/system/lm_sensors.service") {
> +		system("/bin/systemctl", "stop", "lm_sensors.service");
> +	} elsif (-x "/sbin/service" && -f "/etc/init.d/lm_sensors" &&
> +		 -f "/var/lock/subsys/lm_sensors") {
>  		system("/sbin/service", "lm_sensors", "stop");
>  	}
>  

-- 
Jean Delvare

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

Re: [lm-sensors] PATCH: systemd integration

[Hans de Goede]

Hi,

Thanks for the review.

On 04/27/2011 10:56 AM, Jean Delvare wrote:
> Hi Hans,
>
> On Sun, 24 Apr 2011 16:17:16 +0200, Hans de Goede wrote:
>> The attached patch:
>> 1) uses systemd's systemctl to start / stop / enable the lm_sensors service on
>> systemd systems (such as Fedora 15)
>> 2) recognizes the new /run dir for run time info (as used by systemd systems)
>> as a possible place where the udev db can live
>> 3) adds a lm_sensor.service file to install under /lib/systemd/system
>>
>> Please review, I'll push it to svn myself once acked.
>
> I don't know anything about systemd, but the patch looks good and I'm
> all for better integration of upstream lm-sensors so that every
> distribution doesn't have to do it all again.

Yes that was the idea (not every distro having to do it all again,
and also consistent behavior between distros).

>
> Comments:
>
>> Index: prog/init/lm_sensors.service
>> =================================>> --- prog/init/lm_sensors.service	(revision 0)
>> +++ prog/init/lm_sensors.service	(revision 0)
>> @@ -0,0 +1,14 @@
>> +[Unit]
>> +Description=lm_sensors for monitoring motherboard sensor values
>
> lm-sensors is not (or shouldn't be) limited to motherboard sensors.
> Sensors on CPU, memory modules and graphics cards are supported too, and
> hopefully hard disk drive temperatures will be someday too.

Agreed (I didn't write the service file it was contributed by a Fedora user).
>
>> +After=syslog.target
>> +
>> +[Service]
>> +EnvironmentFile=/etc/sysconfig/lm_sensors
>> +Type=oneshot
>> +RemainAfterExit=yes
>> +ExecStart=-/sbin/modprobe -qab $BUS_MODULES $HWMON_MODULES
>> +ExecStart=/usr/bin/sensors -s
>> +ExecStop=-/sbin/modprobe -qabr $BUS_MODULES $HWMON_MODULES
>
> Please keep in mind that both $BUS_MODULES and $HWMON_MODULES may be
> empty, which would cause the above modprobe commands to return an
> error. Still it is legitimate to start the service so that "sensors -s"
> is executed.

Right, AFAIK the  - in front of the command tells systemd to ignore
the return value.

>
>> +
>> +[Install]
>> +WantedBy=multi-user.target
>> Index: prog/detect/sensors-detect
>> =================================>> --- prog/detect/sensors-detect	(revision 5939)
>> +++ prog/detect/sensors-detect	(working copy)
>> @@ -2339,7 +2339,7 @@
>>   	if (!$use_udev) {
>>   		# Try some known default udev db locations, just in case
>>   		if (-e '/dev/.udev.tdb' || -e '/dev/.udev'
>> -		 || -e '/dev/.udevdb') {
>> +		 || -e '/dev/.udevdb' || -e '/run/udev') {
>>   			$use_udev = 1;
>>   			$dev_i2c = '/dev/i2c-';
>>   		}
>> @@ -6378,6 +6378,14 @@
>>   		}
>>   		close(SYSCONFIG);
>>
>> +		if (-x "/bin/systemctl"&&
>> +		    -f "/lib/systemd/system/lm_sensors.service") {
>> +			system("/bin/systemctl", "enable", "lm_sensors.service");
>> +			system("/bin/systemctl", "start", "lm_sensors.service");
>> +			# All done, don't check for /etc/init.d/lm_sensors
>> +			return;
>> +		}
>> +
>>   		print "Copy prog/init/lm_sensors.init to /etc/init.d/lm_sensors\n".
>>   		      "for initialization at boot time.\n"
>>   			unless -f "/etc/init.d/lm_sensors";
>
> Don't you want to display a smiliar message for the systemd case? Users
> installing manually will have to copy prog/init/lm_sensors.service
> to /lib/systemd/system/lm_sensors.service for initialization at boot
> time.

I was going with the assumption that systemd users will get lm_sensors from
their distro. I'll whip up an updated patch adding a check for this.

Regards,

Hans

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