Is there a bug in hotplug.functions w/ ${TYPE}.usermap?

DISCUSSION Heath Elwayne Petersen 4 participants 8 messages Dec 28, 2001 – Dec 31, 2001

Share & tools

Heath Elwayne Petersen

author

2001-12-28 19:37 UTC #0 lore raw

I have been trying to use linux-hotplug to run a script when a device is recognized. However, I have been running into a problem. It seams that there may be an error in the hotplug.functions load_drivers function.

In that function there is a section of code as follows:

	elif find $MODULE_DIR -name $MODULE.o >/dev/null 2>&1 &&
			! $MODPROBE $MODULE >/dev/null 2>&1 ; then
		mesg "... can't load module $MODULE"

It appears that the intent is:

else if the module object file exists
attempt to load the module
if the module load fails
display an error
end-if

If this is the case, the error is that the find command does not return a false return code when it fails to find the file. Therefore, the modprobe will always be tried, and the following code, which attempts to execute /etc/hotplug/$TYPE/$MODULE will never be executed.

I have created an *!*UNTESTED*!* patch to the 2001_09_19 version of hotplug.functions. It is included below. My apologies for not testing it yet, but I've been pulled away on something else.

Please let me know what you think.

Heath Petersen
[email protected]

OUTPUT OF: diff hotplug.functions.orig hotplug.functions ---- Cut Here ----
128c128,129
< elif find $MODULE_DIR -name $MODULE.o >/dev/null 2>&1 &&
---

Quoted 2 lines

          elif find $MODULE_DIR -name $MODULE.o 2>&1 \
                      | grep -q . >/dev/null 2>&1 &&

---- Cut Here ----

_______________________________________________
Linux-hotplug-devel mailing list http://linux-hotplug.sourceforge.net
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Stamatis Mitrofanis

2001-12-29 01:37 UTC #1 lore raw

Quoted 9 lines · Heath Elwayne Petersen

I have been trying to use linux-hotplug to run a script when a device is recognized. However, I have been running into a problem. It seams that there may be an error in the hotplug.functions load_drivers function.
...
If this is the case, the error is that the find command does not return a false return code when it fails to find the file. Therefore, the modprobe will always be tried, and the following code, which attempts to execute /etc/hotplug/$TYPE/$MODULE will never be executed.

To be honest, I find that load_driver part of the code ugly alltogether.
More on that below...

Quoted 11 lines

OUTPUT OF: diff hotplug.functions.orig hotplug.functions ---- Cut Here ----
128c128,129
< elif find $MODULE_DIR -name $MODULE.o >/dev/null 2>&1 &&
---
Quoted 3 lines
         elif find $MODULE_DIR -name $MODULE.o 2>&1 \
                     | grep -q . >/dev/null 2>&1 &&
---- Cut Here ----

Firstly, that `find' line is checking that belongs in modprobe. It's modprobe's job to see if a (abstract) "driver" is available. modprobe has aliases, null/none drivers etc. so it's high-level.

If we're looking for a "module file" we sould be using insmod. The _command_ insmod. Even if it's the same executable as modprobe, it has different semantics.

load_modules is ugly because:
- code is not very unreadable (at least one error went unnoticed)
- it has many story-telling FIXMEs in it for very long (so, it may have a bad architecture)

It's the agent's job to map requests to drivers and modutil's job to load kernel modules. Using a load_driver function between these two is bad because it complicates things.

Why is the requirement for either *modules or the function *_map_modules from load_driver when the mapping can be done by the agent (possibly by calling its helper program)? The agent can simply call modprobe which is definitely high-level-enough. modprobe itself checks whether a driver is already loaded.

For agents that don't map drivers or don't work with kernel drivers at all (net, printer, xfree86) load_modules is of no use.

For driver-specific processing it is (much) better to use scripts as /etc/hotplug/usb/xxx.agent . They should be called in place of (and not in addition to) modprobe so that there can be greater flexibility. (the agent script delegates to a driver script)

I am working with David Brownell for a series of patches that will ease things in this area. I had already solved the problem by the time the previous message was posted!

Keith Owens

2001-12-29 04:38 UTC #2 lore raw

In reply to Heath Elwayne Petersen

Quoted 3 lines · Stamatis Mitrofanis

Firstly, that `find' line is checking that belongs in modprobe. It's modprobe's job to see if a (abstract) "driver" is available. modprobe has aliases, null/none drivers etc. so it's high-level.

modprobe -n foo 2>/dev/null and test the return code. 0 means it found the module, including checking aliases, extra paths etc. You also get 0 if the module is already loaded but that is not a problem.

David Brownell

2001-12-29 18:23 UTC #3 lore raw

In reply to Heath Elwayne Petersen

That sounds like the right fix.

----- Original Message -----
From: "Keith Owens" <[email protected]>
To: "Stamatis Mitrofanis" <[email protected]>
Cc: "linux-hotplug-devel" <[email protected]>
Sent: Friday, December 28, 2001 8:38 PM
Subject: Re: Is there a bug in hotplug.functions w/ ${TYPE}.usermap?

Quoted 4 lines

modprobe -n foo 2>/dev/null and test the return code. 0 means it found the module, including checking aliases, extra paths etc. You also get 0 if the module is already loaded but that is not a problem.

David Brownell

2001-12-29 18:50 UTC #4 lore raw

In reply to Heath Elwayne Petersen

Quoted 2 lines · Stamatis Mitrofanis

load_modules is ugly because:
- code is not very unreadable (at least one error went unnoticed)

"not very unreadable" = double negative = readable ;)

Code generally has bugs, they turn up over time. This one was pretty easily fixed once it turned up.

Quoted 2 lines

- it has many story-telling FIXMEs in it for very long (so, it may have a bad architecture)

FIXME requests that hang out are a sign that people haven't really hit the problem being identified. In this case it's a sign that not many people are using C helper programs to find out what drivers to use ... parsing in BASH seems to work better than some folk thought it would.

Quoted 3 lines

It's the agent's job to map requests to drivers and modutil's job to load kernel modules. Using a load_driver function between these two is bad because it complicates things.

Not all drivers are kernel modules, and load_driver understands that. It also invokes the driver setup scripts.

Quoted 3 lines

Why is the requirement for either *modules or the function *_map_modules from load_driver when the mapping can be done by the agent (possibly by calling its helper program)?

Uh, the agent _is_ doing the mapping, possibly by calling its helper program. It just delegates all the common logic to a helper function, so when there's a bug in the common logic it gets fixed once not N times.

- Dave

Stamatis Mitrofanis

2001-12-30 03:22 UTC #5 lore raw

In reply to Heath Elwayne Petersen

Quoted 2 lines · David Brownell

"not very unreadable" = double negative = readable ;)

Oops... I really hate ugly code though.

Quoted 7 lines

Quoted 4 lines
It's the agent's job to map requests to drivers and modutil's job to load kernel modules. Using a load_driver function between these two is bad because it complicates things.
Not all drivers are kernel modules, and load_driver understands that. It also invokes the driver setup scripts.

Well, not all drivers are drivers, if you ask me. The net agent, for example, doesn't "load" drivers at all. The interface with the kernel describes "events", not directly requests for drivers.

Quoted 5 lines

Quoted 4 lines
Why is the requirement for either *modules or the function *_map_modules from load_driver when the mapping can be done by the agent (possibly by calling its helper program)?
Uh, the agent _is_ doing the mapping, possibly by calling its helper

program.

Quoted 1 line

It just delegates all the common logic to a helper function, so when

there's

Quoted 2 lines

a bug in the common logic it gets fixed once not N times.

There's not much in this "common logic" anyway. The way the function is written makes it seem like there is. It's huge and full of unnecessary things

Shell functions are useful as "emulations" of actual programs. Defining the meaning of *_map_modules doesn't really do much when there can be a shell script implementation of *modules (which is already defined to do the same job).

So off goes the *_map_functions bloat of load_driver . It can simply use *modules .

/****/

Next is a "case" if the LISTER program doesn't exist. If the load_driver function is called,, the *modules "command" _must_ exist. Otherwise, what's the point?

Inside the case there are agent-specific parts. This is the worst place to find agent-specific code. This is a "global function", a "shared logic", so put that code in the _agent scripts_!

So off goes that bloat from load_driver.

/****/

What is left is the ugly-looking part. An amalgam of ifs, elifs, finds etc. that I still can't read, no matter how much I look at them. Here's the actual logic:

for every module i have to load
IF it's already loaded, forget it
IF it's blacklisted, forget it
TRY to load it
IF it's not loaded ok, tell user and forget it TRY to run a module-specific setup script
next module

The "already loaded" part is the job of modprobe, so it's unnecessary.

/****/

So, what's left? Three things.

One "for" loop for every module.
Two driver-specific treatments: the blacklist and the setup script.

The setup script is not flexible because it is called only _after_ the module is loaded, allowing no driver-specific pre-installation.

So, here's my idea:

instead of calling the script _after_ the module is loaded...

_call_the_script_instead_of_loading_the_module_ , and if it doesnt exist, do load the kernel module.

This also gets rid of the need for a "blacklist". There can simply be empty scripts!

/** Bloat removed. **/

So, now, what's left? A "for" and an "if". Really buggy.

I hope you got my point. (whew) The load_module function is not much of an abstraction anyway.

David Brownell

2001-12-30 07:49 UTC #6 lore raw

In reply to Heath Elwayne Petersen

Since you sent that original reply privately, I couldn't see that you had _also_ sent a copy to the list. Please don't do that.

----- Original Message -----
From: "David Brownell" <[email protected]>
To: "Stamatis Mitrofanis" <[email protected]>
Sent: Saturday, December 29, 2001 11:34 PM
Subject: Re: Is there a bug in hotplug.functions w/ ${TYPE}.usermap?

Quoted 55 lines

Quoted 1 line
Oops... I really hate ugly code though.
You're basically complaining because it doesn't match your own coding style. That has nothing to do with being ugly; it's pure flaming. Which doesn't help your cause at all (hint).
Quoted 3 lines
Well, not all drivers are drivers, if you ask me. The net agent, for example, doesn't "load" drivers at all. The interface with the kernel describes "events", not directly requests for drivers.
Yes, hotplugging isn't only about loading drivers. Never has been. If I've ever implied otherwise, it's because I've been spending so much time talking with folk who don't realize that.
Not that the "net" agent talks "driver", it talks "network interface" (device). It's a level above device drivers.
Quoted 3 lines
There's not much in this "common logic" anyway. The way the function is written makes it seem like there is. It's huge and full of unnecessary things
Then we'll just disagree. I've used versions with and without code sharing, and I know which style of agent is smaller. And I've worked with systems designed the way you're advocating (no shared code).
Those are horrible to use or maintain, because every subsystem ends up doing the same things in different ways, and none will quite the same (or right) thing. Users end up hating them because nothing is predictable enough to understand, it's nothing more than a collection of kluges. Maintainers hate them because updates need to touch dozens of parts rather than a single one, and they
only way to add updates is piling kluge on top of kluge. And while
that may not matter in this case, that's the best way for security holes
grow: kluge on kluge, with nothing consistent enough for anyone to know what's going to break when one gets fixed.
Quoted 2 lines
So off goes the *_map_functions bloat of load_driver . It can simply use *modules .
No it can't. "usbdevfs" is completely optional, and most systems don't have either "usbmodules" or "pcimodules". The *_map functions are the required parts, the rest is optional gravy (mostly there to deal with the "coldplug" cases).
Quoted 4 lines
instead of calling the script _after_ the module is loaded...
_call_the_script_instead_of_loading_the_module_ , and if it doesnt exist, do load the kernel module.
I've thought about that before, but it's got some problems too.

Quoted 9 lines

For one, it'd break existing scripts, and (often worse) knowledge that folk have already about using them. For another, there's no compelling example motivating such a change -- no problem that folk have hit (real-world) that doesn't have a good solution within the existing framework.
- Dave

Stamatis Mitrofanis

2001-12-31 04:42 UTC #7 lore raw

In reply to Heath Elwayne Petersen

Quoted 8 lines · Stamatis Mitrofanis

Quoted 3 lines · David Brownell
Since you sent that original reply privately, I couldn't see that you had _also_ sent a copy to the list. Please don't do that.
I neglected to add the CC: header and, when I noticed, it was already too late.

I did the same mistake again!!