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

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

[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.

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
HeathPetersen@CompuServe.com

OUTPUT OF: diff hotplug.functions.orig hotplug.functions
---- Cut Here ----
128c128,129
<           elif find $MODULE_DIR -name $MODULE.o >/dev/null 2>&1 &&
---
>           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
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

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

[Stamatis Mitrofanis]

Heath Elwayne Petersen wrote:

>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...

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


_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

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

[Keith Owens]

On Sat, 29 Dec 2001 03:37:22 +0200, 
Stamatis Mitrofanis <ewstam@softhome.net> wrote:
>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.


_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

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

[David Brownell]

That sounds like the right fix.

----- Original Message ----- 
From: "Keith Owens" <kaos@ocs.com.au>
To: "Stamatis Mitrofanis" <ewstam@softhome.net>
Cc: "linux-hotplug-devel" <linux-hotplug-devel@lists.sourceforge.net>
Sent: Friday, December 28, 2001 8:38 PM
Subject: Re: Is there a bug in hotplug.functions w/ ${TYPE}.usermap?


> 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.
> 


_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

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

[David Brownell]

Stamatis Mitrofanis wrote:

> 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.


> - 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.


> 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.


> 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



_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

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

[Stamatis Mitrofanis]

David Brownell wrote:

 >"not very unreadable" = double negative = readable ;)
 >
Oops... I really hate ugly code though.

 >>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.

 >>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.
 >
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.



_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

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

[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.


----- Original Message ----- 
From: "David Brownell" <david-b@pacbell.net>
To: "Stamatis Mitrofanis" <ewstam@softhome.net>
Sent: Saturday, December 29, 2001 11:34 PM
Subject: Re: Is there a bug in hotplug.functions w/ ${TYPE}.usermap?


> > 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).
> 
> 
> > 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.
> 
> 
> > 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.
> 
> 
> > 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).
> 
> 
> > 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.
> 
> 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
> 
> 


_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

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

[Stamatis Mitrofanis]

Stamatis Mitrofanis wrote:

> David Brownell wrote:
>
>> 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!!


_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel