Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!

Bug 480018

Summary: sys-apps/lm_sensors-3.3.4: /etc/init.d/lm_sensors should not check for MODULES_0 in /etc/conf.d/lm_sensors
Product: Gentoo Linux Reporter: Eugene Ho <blasterol>
Component: Current packagesAssignee: Thomas Deutschmann <whissi>
Status: RESOLVED FIXED    
Severity: normal CC: anton.wd, axiator, erhard_f, esigra, henning, ironiridis, n-roeser, nathan.waivio, openrc, paul, polynomial-c, spideybr, tobias.pal, walch.martin, whissi, zeekec
Priority: Normal    
Version: unspecified   
Hardware: All   
OS: Linux   
Whiteboard:
Package list:
Runtime testing required: ---
Attachments: Proposed Patch
initd patch
initd patch
files/lm_sensors-3-r1-init.d
files/lm_sensors-3-r1-conf.d
files/lm_sensors-3.4.0-sensors-detect-gentoo.patch
lm_sensors-3.4.0-r1.ebuild

Description Eugene Ho 2013-08-07 08:04:09 UTC
As of upstream commit 6115 <http://lm-sensors.org/changeset/6115/lm-sensors/trunk>, sensors-detect no longer generates MODULE_0, MODULE_1, MODULE_2 variables.

However, /etc/init.d/lm_sensors as supplied by sys-apps/lm_sensors-3.3.4 still checks for MODULE_0 in the case that modules are to be loaded. Since this variable does not exist anymore, attempting to start the lm_sensors service fails with a "MODULE_0 is not set in /etc/conf.d/lm_sensors, try running sensors-detect" error, even after running sensors-detect.

Should this be changed to check for HWMON_MODULES instead?

Reproducible: Always
Comment 1 Claudio Roberto Fran├ža Pereira 2013-12-25 18:11:51 UTC
I confirm the bug in lm_sensors-3.3.4-r1.
I worked around it defining MODULE_0 with the same value as HWMON_MODULES (just one module here).
Comment 2 avx 2014-12-18 00:18:43 UTC
This should really be an easy fix, yet nearly one year later, this bug still hit me today. Any reason this isn't "fixed", yet?
Comment 3 Opportunist 2015-02-21 09:03:05 UTC
Confirm on AMD64
Comment 4 Nathan 2015-03-27 16:09:25 UTC
Created attachment 399878 [details, diff]
Proposed Patch

First ever time writting shell script and submitting patch.
I'm not sure if the script is portable or follows any coding standards.
I tried to make it backwards compatable with MODULE_0 because MODULE_ way of doing things allows you to specify arguments to pass to the module.

I didn't change any copywrite date or anything, I don't know if I needed to do that.

It worked on my system.
Comment 5 Anton Kuleshov 2015-03-31 03:53:59 UTC
Created attachment 400244 [details, diff]
initd patch

initd patch for version 3.3.4 and above. Must work with version 3.3.3 which is also in the tree.
Comment 6 Anton Kuleshov 2015-03-31 04:32:57 UTC
Sorry, did not see prev patch comment. I think MODULE_X variables support not needed, because it exist only for compatibility reasons and was removed from >=3.3.4. No sense if 3.3.3 version will be removed from tree in close future.
Comment 7 Henning Schild 2015-04-07 14:01:38 UTC
Both proposed patches do not handle BUS_MODULES and therefore only solve half of the problem.

In gentoo we patch lm_sensors to modify the output with -sensors-detect-gentoo.patch. I suggest to revert http://lm-sensors.org/changeset/6115/lm-sensors/trunk as part of that patch and just keep the old init scripts.
Comment 8 Anton Kuleshov 2015-04-07 14:41:43 UTC
BUS_MODULES is optional because all module dependency must be loaded automatically. Maybe im wrong. Anyway, must be simple to add support for it too.
Comment 9 Anton Kuleshov 2015-04-07 14:48:21 UTC
Created attachment 400766 [details, diff]
initd patch
Comment 10 Henning Schild 2015-04-08 08:49:32 UTC
One loop with concatenated module lists is enough, but even then i do not think that is very elegant. Thinking about it the whole module loading should be done by suggesting people to modify /etc/conf.d/modules. That should be the sensors-detect patch gentoo should apply, and the lm_sensors init script would get a "after modules".
Comment 11 Henning Schild 2015-04-08 09:00:06 UTC
Just tried it ...

/etc/init./lm_sensors:
...
> LOADMODULES=no
...

/etc/conf.d/modules:
...
lm_sensors_modules="coretemp"
modules="$modules $lm_sensors_modules"

added the "after modules" dep to /etc/init./lm_sensors

and now run "/etc/init.d/modules zap; /etc/init.d/modules start", as a way to load you new additional modules without unloading the old ones.
Comment 12 Nathan 2015-04-10 00:47:03 UTC
Anton's patch works great on my system.
It seems like a good idea to get rid of MODULES_X.
Addition of ${BUS_MODULES} makes it much more like upstream's lm_sensors.init
Comment 13 Nathan 2015-04-10 00:56:45 UTC
Comment on attachment 399878 [details, diff]
Proposed Patch

>--- orig.lm_sensors-3-init.d	2008-03-17 02:59:28.000000000 -0500
>+++ lm_sensors-3-init.d	2015-03-27 11:02:28.003256446 -0500
>@@ -10,8 +10,12 @@
> 	fi
> 
> 	if [ "${LOADMODULES}" = "yes" -a -f /proc/modules ]; then
>-		if [ -z "${MODULE_0}" ]; then
>-			eerror "MODULE_0 is not set in /etc/conf.d/lm_sensors, try running sensors-detect"
>+		if [ -z "${MODULE_0}" ] && [ -z "${HWMON_MODULES}" ]; then
>+			eerror "HWMON_MODULES or MODULE_0 is not set in /etc/conf.d/lm_sensors, try running sensors-detect"
>+			return 1
>+		fi
>+		if ! [ -z "${MODULE_0}" ] && ! [ -z "${HWMON_MODULES}" ]; then
>+			eerror "HWMON_MODULES or MODULE_0 can't be both set in /etc/conf.d/lm_sensors, try running sensors-detect"
> 			return 1
> 		fi
> 	fi
>@@ -46,18 +50,28 @@
> 			eend 0
> 		fi
> 
>-		i=0
>-		while true; do
>-			module=`eval echo '$'MODULE_${i}`
>-			module_args=`eval echo '$'MODULE_${i}_ARGS`
>-			if [ -z "${module}" ]; then
>-				break
>-			fi
>-			ebegin "  Loading ${module}"
>-			modprobe ${module} ${module_args} >/dev/null 2>&1
>-			eend $?
>-			i=$(($i+1))
>-		done
>+		if ! [ -z "${MODULE_0}" ]; then
>+			i=0
>+			while true; do
>+				module=`eval echo '$'MODULE_${i}`
>+				module_args=`eval echo '$'MODULE_${i}_ARGS`
>+				if [ -z "${module}" ]; then
>+					break
>+				fi
>+				ebegin "  Loading ${module}"
>+				modprobe ${module} ${module_args} >/dev/null 2>&1
>+				eend $?
>+				i=$(($i+1))
>+			done
>+		fi
>+
>+		if ! [ -z "${HWMON_MODULES}" ]; then
>+			for module in ${HWMON_MODULES}; do
>+				ebegin "  Loading ${module}"
>+				modprobe ${module} >/dev/null 2>&1
>+				eend $?
>+			done
>+		fi
> 	fi
> 
> 	if [ "${INITSENSORS}" = "yes" ]; then
>@@ -78,23 +92,39 @@
> 	if [ "${LOADMODULES}" = "yes" -a -f /proc/modules ]; then
> 		einfo "Unloading lm_sensors modules..."
> 
>-		# find the highest possible MODULE_ number
>-		i=0
>-		while true; do
>-			module=`eval echo '$'MODULE_${i}`
>-			if [ -z "${module}" ] ; then
>-				break
>-			fi
>-			i=$(($i+1))
>-		done
>+		if ! [ -z "${MODULE_0}" ]; then
>+			# find the highest possible MODULE_ number
>+			i=0
>+			while true; do
>+				module=`eval echo '$'MODULE_${i}`
>+				if [ -z "${module}" ] ; then
>+					break
>+				fi
>+				i=$(($i+1))
>+			done
> 
>-		while [ ${i} -gt 0 ]; do
>-			i=$(($i-1))
>-			module=`eval echo '$'MODULE_${i}`
>-			ebegin "  Unloading ${module}"
>-			rmmod ${module} >/dev/null 2>&1
>-			eend $?
>-		done
>+			while [ ${i} -gt 0 ]; do
>+				i=$(($i-1))
>+				module=`eval echo '$'MODULE_${i}`
>+				ebegin "  Unloading ${module}"
>+				rmmod ${module} >/dev/null 2>&1
>+				eend $?
>+			done
>+		fi
>+
>+		if ! [ -z "${HWMON_MODULES}" ]; then
>+			# unload modules in reverse order
>+			REV_HW_MODULES=""
>+			for module in ${HWMON_MODULES}; do
>+				REV_HWMON_MODULES=${module}" "${REV_HWMON_MODULES}
>+			done
>+
>+			for module in ${REV_HWMON_MODULES}; do
>+				ebegin "  Unloading ${module}"
>+				rmmod ${module} >/dev/null 2>&1
>+				eend $?
>+			done
>+		fi
> 
> 		if [ -e /proc/sys/dev/sensors ] ; then
> 			ebegin "  Unloading i2c-proc"
Comment 14 Henning Schild 2015-04-10 08:55:04 UTC
The gentoo way of loading modules is to use /etc/init.d/modules. And since we have to change the lm_sensors init script i propose to move to the existing infrastructure and not write "modprobe" and "rmmod" loops again and again.
Moving to modules would result in a much shorter init script for lm_sensors, all the module related code would be turned into a "after modules" dep. The whole init script will boil down to an optional "sensors -s", see INIT_SENSORS.

We would however loose the possibilty to unload modules right when we stop lm_sensors. I actually do not know what this feature is good for. init.d/modules does not care about unloading. Running a rmmod loop will just slow down your system shutdown. Does anyone have a convincing argument for the rmmod loop in the stop function?

Moving to modules would mean we have to patch the sensors-detect tool of lm_sensors to suggest config changes that will work in /etc/conf.d/modules. Gentoo already patches exactly that perl script, no extra effort, just different.

We probably need a enews to tell users about the change.

That is an RFC so please let me knot what you think, i might send patches. But it would be nice to hear about major issues i overlooked.
Comment 15 Henning Schild 2015-08-21 14:15:08 UTC
Created attachment 409754 [details]
files/lm_sensors-3-r1-init.d
Comment 16 Henning Schild 2015-08-21 14:15:44 UTC
Created attachment 409756 [details]
files/lm_sensors-3-r1-conf.d
Comment 17 Henning Schild 2015-08-21 14:16:21 UTC
Created attachment 409758 [details, diff]
files/lm_sensors-3.4.0-sensors-detect-gentoo.patch
Comment 18 Henning Schild 2015-08-21 14:17:04 UTC
Created attachment 409760 [details]
lm_sensors-3.4.0-r1.ebuild
Comment 19 Henning Schild 2015-08-21 14:23:31 UTC
Guys. Please give this ebuild a try. It offloads the module-loading to openrc and will assist the user in changing /etc/conf.d/modules.

That really simplifies the init script in a nice way. But the change is kind of drastic and will require a news item or some yellow/red color in the einfo. But seeing dispatch-conf messing with the init script and conf will give users an idea as well ...

How are the modules handled in the systemd case? Seems i did not brake anything unless systemd reused the old openrc script.
Comment 20 Ryan Hill (RETIRED) gentoo-dev 2015-11-15 05:59:03 UTC
CCing openrc.
Comment 21 Christohper Harrington 2016-02-25 22:57:49 UTC
Is there something holding up the change mentioned in comment 18? It's 6 months old.
Comment 22 Henning Schild 2016-02-26 08:56:07 UTC
I have no idea, maybe these days one needs to propose something like that as a github pull request to get attention. Christopher maybe you can try it and comment on how it worked for you.
Comment 23 Henning Schild 2016-02-26 09:12:11 UTC
If changing /etc/conf.d/modules seems too intrusive we could apply the symlink scheme used for /etc/init.d/net.* and eventually have /etc/(conf.d|init.d)/modules.lm_sensors and have /etc/init.d/lm_sensors depend on modules.lm_sensors.

But that might require changing openrc to support the symlink scheme on the modules init-file. Something that might help simplify other init-scripts that deal with loading modules as well. But before going that way i would prefer getting a review an the current proposal.
Comment 24 Regna 2016-03-15 04:29:04 UTC
Hello all.

I agree, using /etc/conf.d/modules is a right way to load modules, instead of rewriting module-loading loops over and over. It may be more convenient to use multiple files like conf.d/modules.name or /etc/modules.d, but the current approach will also do it's job. I've tested this ebuild, everything seems to be working flawlessly.

This so-hard-to-fix bug is almost three years old, let's do something about it.
Comment 25 Thomas Deutschmann gentoo-dev Security 2016-03-15 12:43:19 UTC
I am currently working on this bug as part of the Gentoo Proxy-Maintainers project. Should be resolved within this week.
Comment 26 Christohper Harrington 2016-03-27 23:17:27 UTC
(In reply to Thomas D. from comment #25)
> I am currently working on this bug as part of the Gentoo Proxy-Maintainers
> project. Should be resolved within this week.

Pinging.
Comment 27 Thomas Deutschmann gentoo-dev Security 2016-03-31 23:39:04 UTC
This is in progress however I need this weekend to run more tests: I am not just fixing this bug, I am also working on the other open bugs of this package.

Thank you for your patience.
Comment 28 WOLfgang Schricker 2016-05-19 10:50:41 UTC
sys-apps/lm_sensors-3.4.0-r1::x-portage works great!
Good patch set, good work. THX
Comment 29 Christohper Harrington 2016-06-07 14:17:08 UTC
What is currently blocking progress on this bug?

Can I offer any help? I don't want to duplicate work, and I also don't understand what is technically deficient with the solution from Henning Schild (comment 18).
Comment 30 Christohper Harrington 2016-08-06 21:39:30 UTC
Given the state of the package as broken-out-of-the-box, should we simply mask the package to actually figure out if someone is using it?

Or, alternately, just remove /etc/init.d/lm_sensors ? As far as I can tell, the only significant role it plays is to load modules. Maybe it would be easier to patch sensors-detect to output a list of module names to add to /etc/conf.d/modules ...
Comment 31 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2016-08-08 20:37:14 UTC
(In reply to Christohper Harrington from comment #30)
> Given the state of the package as broken-out-of-the-box, should we simply
> mask the package to actually figure out if someone is using it?
If you have an already working conf.d/lm_sensors it continues to work fine.

> Or, alternately, just remove /etc/init.d/lm_sensors ? As far as I can tell,
> the only significant role it plays is to load modules. Maybe it would be
> easier to patch sensors-detect to output a list of module names to add to
> /etc/conf.d/modules ...
It also runs "sensors -s", which is crucial on some systems (without it my fans run full blast).

whissi: you're a dev now, please go ahead and fix it, in such a way that doesn't break people with old conf.d/lm_sensors.
Comment 32 Thomas Deutschmann gentoo-dev Security 2016-08-08 20:53:57 UTC
I am currently patching sensors-detect the following way:

For systemd:
It asks the user if it should generate or overwrite "/etc/modules-load.d/lm_sensors.conf".

For OpenRC:
It asks the user if it should add a block like

> # BEGIN sys-apps/lm_sensors
> # Generated by sensors-detect on Mon Aug  8 22:37:03 2016
> modules="$modules coretemp"
> # END sys-apps/lm_sensors

to "/etc/conf.d/modules" but _only_ if the user doesn't have set modules for a specific kernel version (i.e. 'modules_2="ipv6"'). In that case we will tell the user that he/she has to manually add the modules.

This block can be overwritten if you re-run sensors-detect (if the marker wasn't touched).

I am not sure what to do with existing configurations. I'd prefer that existing user will have to migrate their configurations (i.e. moving modules from "/etc/conf.d/lm_sensors" to "/etc/conf.d/modules"). This would allow us to keep the new runscript clean.

Objections?
Comment 33 Robin Johnson archtester Gentoo Infrastructure gentoo-dev Security 2016-08-08 21:50:50 UTC
(In reply to Thomas Deutschmann from comment #32)
> I am currently patching sensors-detect the following way:
...
> I am not sure what to do with existing configurations. I'd prefer that
> existing user will have to migrate their configurations (i.e. moving modules
> from "/etc/conf.d/lm_sensors" to "/etc/conf.d/modules"). This would allow us
> to keep the new runscript clean.
> 
> Objections?
Make sure you can convert module parameters as well.
In my case, the w83627ehf module, needs fan_debounce=1. It used to need force_id as well, but that got fixed.
Comment 34 Thomas Deutschmann gentoo-dev Security 2016-08-10 10:31:33 UTC
My changes are currently beeing reviewed.

If you don't want to wait or want to test too feel free to grab the changes from my overlay at https://git.io/v6WX4.


Summary of changes:

  - Based on latest unreleased changes (snapshot release from 2016-07-25;
    updated/added detection for Fintek F81768, Nuvoton NCT6793D, Microchip
    MCP9808 and Mark F71868A chips; various fixes for sensors-detect, see
    https://git.io/v6Wig for all changes)

  - sensors-detect now writes to "/etc/conf.d/modules" (OpenRC) or
    "/etc/modules-load.d/lm_sensors.conf" (systemd) and uses a config file
    protection mechanism like known from emerge (i.e. after making changes
    you have to call tools such as dispatch-conf, cfg-update or etc-update
    to merge the changes; new "--no-gentoo-config-protect" parameter will
    disable config file protection)
    
    Gentoo-specific changes can be reviewed from https://git.io/v6WXq

  - Ebuild is now multilib compatible (see bug 529684)

  - Runscripts no longer passes config file as argument. While this removes
    the possibility to use different configurations per default services it
    allows you to use "/etc/sensors.d" (see bug 490502 and bug 307249)

  - Runscripts improved in general. I.e. if you start fancontrol service
    without a valid config (so that fancontrol doesn't start and exit with
    an error) you will now know at least, that the service didn't start.
    Now you will also have reliable status functions to know service state.

  - lm_service service no longer loads modules. You have to use mechanism
    your init system provides to do that (that's the scope of this bug)


Thank you for your patience.
Comment 35 Thomas Deutschmann gentoo-dev Security 2016-09-16 12:39:48 UTC
Again, thank you for you patience. We had to delay lm_sensors due to some problems in OpenRC's new modules-load service but now everything is ready and in tree:

> commit 9ba6c0d353dd2ad3936fe35095588648b472f188
> Author: Thomas Deutschmann
> Date:   Fri Sep 16 14:36:11 2016 +0200
> 
>     sys-apps/lm_sensors: Version bump to snapshot release from 2016-07-25; Package now uses module-load service
> 
>     Changes:
>     ========
>      - Based on latest unreleased changes (snapshot release from 2016-07-25;
>        updated/added detection for Fintek F81768, Nuvoton NCT6793D, Microchip
>        MCP9808 and Mark F71868A chips; various fixes for sensors-detect, see
>        https://git.io/v6Wig for all changes)
> 
>      - sensors-detect now writes to "/etc/modules-load.d/lm_sensors.conf" and
>        uses a config file protection mechanism like known from emerge (i.e.
>        after making changes you have to call tools such as dispatch-conf,
>        cfg-update or etc-update to merge the changes;
>        new "--no-gentoo-config-protect" parameter will disable config file
>        protection)
> 
>      - Due to previous change lm_service service no longer loads modules. You
>        have to use mechanism your init system provides to do that (i.e. make
>        sure your init system's modules-load service is enabled).
> 
>      - Ebuild is now multilib compatible (see bug 529684)
> 
>      - Runscripts no longer passes config file as argument. While this removes
>        the possibility to use different configurations per default services it
>        allows you to use "/etc/sensors.d" (see bug 490502 and bug 307249)
> 
>      - Runscripts improved in general. I.e. if you start fancontrol service
>        without a valid config (so that fancontrol doesn't start and exit with
>        an error) you will now know at least, that the service didn't start.
>        Now you will also have reliable status functions to know service state.
> 
>     Gentoo-Bug: https://bugs.gentoo.org/592916
>     Gentoo-Bug: https://bugs.gentoo.org/529684
>     Gentoo-Bug: https://bugs.gentoo.org/490502
>     Gentoo-Bug: https://bugs.gentoo.org/307249
> 
>     Package-Manager: portage-2.3.0
Comment 36 ernsteiswuerfel 2016-09-16 18:24:00 UTC
Many thanks for your efforts!