From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stamatis Mitrofanis Date: Sun, 30 Dec 2001 03:22:34 +0000 Subject: Re: Is there a bug in hotplug.functions w/ ${TYPE}.usermap? Message-Id: List-Id: References: In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-hotplug@vger.kernel.org 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