obsolete modparam change busted.

obsolete modparam change busted.

[Dave Jones]

Circa 2.6.10, the module loader started barfing if
modprobe.conf contained obsolete parameters.

However this change was broken, and if the modprobe.conf
has trailing whitespace, modules fail to load with the
following helpful message..

snd_intel8x0: Unknown parameter `'

This ends up screwing over people who upgrade from previously
working configurations, so we've been backing out that change
in Fedora for a while with the patch below.

It doesn't look like the right thing to do, but it has got
things working again at least. Probably we should just check
explicity for whitespace and ignore it somewhere else in
the module loader.  Rusty?

		Dave


diff -urNp --exclude-from=/home/davej/.exclude linux-1503/kernel/module.c linux-1700/kernel/module.c
--- linux-1503/kernel/module.c
+++ linux-1700/kernel/module.c
@@ -1707,8 +1707,6 @@ static struct module *load_module(void _
 				 / sizeof(struct kernel_param),
 				 NULL);
 	}
-	if (err < 0)
-		goto arch_cleanup;
 
 	err = mod_sysfs_setup(mod, 
 			      (struct kernel_param *)

Re: obsolete modparam change busted.

[Rusty Russell]

On Mon, 2005-08-08 at 14:49 -0400, Dave Jones wrote:
> However this change was broken, and if the modprobe.conf
> has trailing whitespace, modules fail to load with the
> following helpful message..

Hi Dave,

	This fix should be preferable, I think.

Name: Ignore trailing whitespace on kernel parameters correctly
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Dave Jones says:

... if the modprobe.conf has trailing whitespace, modules fail to load
with the following helpful message..

	snd_intel8x0: Unknown parameter `'

--- linux-2.6.12/kernel/params.c	2005-07-15 04:39:53.000000000 +1000
+++ /tmp/foo.c	2005-08-09 13:56:04.000000000 +1000
@@ -1,0 +1,0 @@
@@ -135,11 +80,8 @@
 
 	DEBUGP("Parsing ARGS: %s\n", args);
 
-	while (*args) {
-		int ret;
-
-		args = next_arg(args, &param, &val);
-		ret = parse_one(param, val, params, num, unknown);
+	while (*(args = next_arg(args, &param, &val))) {
+		int ret = parse_one(param, val, params, num, unknown);
 		switch (ret) {
 		case -ENOENT:
 			printk(KERN_ERR "%s: Unknown parameter `%s'\n",

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: obsolete modparam change busted.

[Dave Jones]

On Tue, Aug 09, 2005 at 02:01:16PM +1000, Rusty Russell wrote:
 > On Mon, 2005-08-08 at 14:49 -0400, Dave Jones wrote:
 > > However this change was broken, and if the modprobe.conf
 > > has trailing whitespace, modules fail to load with the
 > > following helpful message..
 > 
 > Hi Dave,
 > 
 > 	This fix should be preferable, I think.
 > 
 > Name: Ignore trailing whitespace on kernel parameters correctly
 > Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>


Hey Rusty,
 This seems to have introduced a different regression :-)

https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=165633

We're now munching the end of the boot command line it seems.

		Dave

Re: obsolete modparam change busted.

[Rusty Russell]

On Sat, 2005-08-13 at 14:27 -0400, Dave Jones wrote:
> We're now munching the end of the boot command line it seems.

Wow, if we had testcases in the kernel source, I would not have to keep
rewriting them (badly) every time I touched this code.

Throw away that stupid patch, apply this stupid patch.


Name: Ignore trailing whitespace on kernel parameters correctly: Fixed version
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

Dave Jones says:

... if the modprobe.conf has trailing whitespace, modules fail to load
with the following helpful message..

	snd_intel8x0: Unknown parameter `'

Previous version truncated last argument.

Index: linux-2.6.13-rc6-git7-Module/kernel/params.c
===================================================================
--- linux-2.6.13-rc6-git7-Module.orig/kernel/params.c	2005-08-10 16:12:45.000000000 +1000
+++ linux-2.6.13-rc6-git7-Module/kernel/params.c	2005-08-16 14:31:16.000000000 +1000
@@ -80,8 +80,6 @@
 	int in_quote = 0, quoted = 0;
 	char *next;
 
-	/* Chew any extra spaces */
-	while (*args == ' ') args++;
 	if (*args == '"') {
 		args++;
 		in_quote = 1;
@@ -121,6 +119,9 @@
 		next = args + i + 1;
 	} else
 		next = args + i;
+
+	/* Chew up trailing spaces. */
+	while (*next == ' ') next++;
 	return next;
 }
 
@@ -134,6 +135,9 @@
 	char *param, *val;
 
 	DEBUGP("Parsing ARGS: %s\n", args);
+	
+	/* Chew leading spaces */
+	while (*args == ' ') args++;
 
 	while (*args) {
 		int ret;

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman

Re: obsolete modparam change busted.

[Dave Jones]

On Tue, Aug 16, 2005 at 02:39:10PM +1000, Rusty Russell wrote:
 > On Sat, 2005-08-13 at 14:27 -0400, Dave Jones wrote:
 > > We're now munching the end of the boot command line it seems.
 > 
 > Wow, if we had testcases in the kernel source, I would not have to keep
 > rewriting them (badly) every time I touched this code.

Maybe someone should write a userspace test harness for kernel code. :-P

 > Throw away that stupid patch, apply this stupid patch.

I'll throw it into the Fedora kernel tomorrow, and re-test.
Thanks for chasing this so quickly.

		Dave