kernel param and KBUILD_MODNAME name-munging mess

kernel param and KBUILD_MODNAME name-munging mess

[Mikael Pettersson]

Booting kernel 2.5.59 with the "-s" kernel boot parameter
doesn't get you into single-user mode like it should.

One part of the problem is Rusty's new module option and
kernel boot parameter parsing code, which changes '-' to
'_' in every string. This change is not reverted when the
string is found NOT to be a kernel option, with the result
that "_s" is passed to init instead of "-s".

Why is this s/-/_/ stuff done at all?
I suppose it's because the string is compared with
MODULE_PARAM_PREFIX, which is __stringify(KBUILD_MODNAME) ".",
and KBUILD_MODNAME is the module name after s/-/_/.

And why does KBUILD_MODNAME change the name like this?
A grep for KBUILD_MODNAME shows that it's only used in #ifdef
and __stringify(). __stringify() macro-expands its argument
before turning it to a string literal. If this is intensional,
then its a bug, since it breaks module parameters to any module
whose (munged) name also is a #define. (I was bitten by that
myself recently when adding a module param to ide-scsi.c.)

Would anything break if we made scripts/Makefile.lib set
KBUILD_MODNAME to the original module name in "", drop the
__stringify() around uses of KBUILD_MODNAME, and remove the
s/-/_/ from kernel/param.c ?

/Mikael

Re: kernel param and KBUILD_MODNAME name-munging mess

[Vamsi Krishna S.]

On Mon, 20 Jan 2003 19:12:52 +0530, Mikael Pettersson wrote:

> And why does KBUILD_MODNAME change the name like this? A grep for KBUILD_MODNAME
> shows that it's only used in #ifdef and __stringify(). __stringify()
> macro-expands its argument before turning it to a string literal. If this is
> intensional, then its a bug, since it breaks module parameters to any module
> whose (munged) name also is a #define. (I was bitten by that myself recently
> when adding a module param to ide-scsi.c.)
> 
> Would anything break if we made scripts/Makefile.lib set KBUILD_MODNAME to the
> original module name in "", drop the __stringify() around uses of
> KBUILD_MODNAME, and remove the s/-/_/ from kernel/param.c ?
> 
This is what Rusty had to say regd s/-/_/ for KBUILD_MODNAME.

--Vamsi.

--quote--
From: Rusty Russell <rusty@rustcorp.com.au>
To: Zwane Mwaikambo <zwane@holomorphy.com>
Cc: vamsi@in.ibm.com, lkml <linux-kernel@vger.kernel.org>
Subject: Re: [BUG] module-init-tools 0.9.3, rmmod modules with '-'
Date: Tue, 17 Dec 2002 11:17:05 +1100

In message <Pine.LNX.4.50.0212161831340.1804-100000@montezuma.mastecende.com> y
ou write:
> On Tue, 17 Dec 2002, Rusty Russell wrote:
>
> > How did you get a module which has - in its name?  The build system
> > *should* turn them into _'s.
>
> ALSA modules?
>
> -rw-r--r--    1 root     root       170125 Dec 15 00:10 snd-mixer-oss.ko
> -rw-r--r--    1 root     root       143685 Dec 15 00:10 snd-mpu401-uart.ko
> -rw-r--r--    1 root     root       312564 Dec 15 00:10 snd-opl3-lib.ko
> -rw-r--r--    1 root     root       194307 Dec 15 00:10 snd-opl3sa2.ko
> -rw-r--r--    1 root     root       612512 Dec 15 00:10 snd-opl3-synth.ko
> -rw-r--r--    1 root     root      1160272 Dec 15 00:10 snd-pcm.ko
>
> But they do get converted when we load ie snd-pcm turns into snd_pcm

Yes, the filenames are unchanged.  But if you modprobe snd-mixer-oss,
you'll see snd_mixer_oss in /proc/modules.  But rmmod "snd-mixer-oss"
works as expected.  Basically, the kernel and tools see them as
equivalent: anything else is a bug, please report.

BTW, this was done for (1) simplicity, (2) so KBUILD_MODNAME can be
used to construct identifiers, and (3) so parameters when the module
is built-in have a consistent name.

Hope that clarifies!

Re: kernel param and KBUILD_MODNAME name-munging mess

[Rusty Russell]

In message <200301201341.OAA23795@harpo.it.uu.se> you write:
> Booting kernel 2.5.59 with the "-s" kernel boot parameter
> doesn't get you into single-user mode like it should.
> 
> One part of the problem is Rusty's new module option and
> kernel boot parameter parsing code, which changes '-' to
> '_' in every string. This change is not reverted when the
> string is found NOT to be a kernel option, with the result
> that "_s" is passed to init instead of "-s".

That's a bug.  Good catch.  Fix below.

> Why is this s/-/_/ stuff done at all?
> I suppose it's because the string is compared with
> MODULE_PARAM_PREFIX, which is __stringify(KBUILD_MODNAME) ".",
> and KBUILD_MODNAME is the module name after s/-/_/.

Basically because making - and _ equal is the only sane way to unify
parameter names without pissing off users, and life for coders is much
nicer when KBUILD_MODNAME is a unique valid C prefix for that module.

Thanks for the report!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Avoid mangling - in parameters
Author: Rusty Russell
Status: Trivial (tested in userspace framework)

D: Mikael Pettersson points out that "-s" gets mangled to "_s" on the
D: kernel command line, even though it turns out not to be a
D: parameter.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.59/kernel/params.c working-2.5.59-underscore/kernel/params.c
--- linux-2.5.59/kernel/params.c	2003-01-02 14:48:01.000000000 +1100
+++ working-2.5.59-underscore/kernel/params.c	2003-01-21 18:16:05.000000000 +1100
@@ -27,6 +27,22 @@
 #define DEBUGP(fmt, a...)
 #endif
 
+static inline int dash2underscore(char c)
+{
+	if (c == '-')
+		return '_';
+	return c;
+}
+
+static inline int parameq(const char *input, const char *paramname)
+{
+	unsigned int i;
+	for (i = 0; dash2underscore(input[i]) == paramname[i]; i++)
+		if (input[i] == '\0')
+			return 1;
+	return 0;
+}
+
 static int parse_one(char *param,
 		     char *val,
 		     struct kernel_param *params, 
@@ -37,7 +53,7 @@ static int parse_one(char *param,
 
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
-		if (strcmp(param, params[i].name) == 0) {
+		if (parameq(param, params[i].name)) {
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);
 			return params[i].set(val, &params[i]);
@@ -69,8 +85,6 @@ static char *next_arg(char *args, char *
 		if (equals == 0) {
 			if (args[i] == '=')
 				equals = i;
-			else if (args[i] == '-')
-				args[i] = '_';
 		}
 		if (args[i] == '"')
 			in_quote = !in_quote;

Re: kernel param and KBUILD_MODNAME name-munging mess

[Ingo Oeser]

On Mon, Jan 20, 2003 at 02:41:03PM +0100, Mikael Pettersson wrote:
> Booting kernel 2.5.59 with the "-s" kernel boot parameter
> doesn't get you into single-user mode like it should.

Try using "s" instead. This works since ever. I didn't even know,
that the other option exists, too.

Regards

Ingo Oeser
-- 
Science is what we can tell a computer. Art is everything else. --- D.E.Knuth

Re: kernel param and KBUILD_MODNAME name-munging mess

[Mikael Pettersson]

Ingo Oeser writes:
 > On Mon, Jan 20, 2003 at 02:41:03PM +0100, Mikael Pettersson wrote:
 > > Booting kernel 2.5.59 with the "-s" kernel boot parameter
 > > doesn't get you into single-user mode like it should.
 > 
 > Try using "s" instead. This works since ever. I didn't even know,
 > that the other option exists, too.

That's a workaround for this particular case, but the name-munging
is still wrong and broken.

With "foo-bar=fie-fum" passed to the kernel, "foo_bar=fie-fum" is
what's put into init's environment. (I checked.)

Re: kernel param and KBUILD_MODNAME name-munging mess

[Alex Riesen]

Mikael Pettersson, Wed, Jan 22, 2003 11:20:01 +0100:
> Ingo Oeser writes:
>  > On Mon, Jan 20, 2003 at 02:41:03PM +0100, Mikael Pettersson wrote:
>  > > Booting kernel 2.5.59 with the "-s" kernel boot parameter
>  > > doesn't get you into single-user mode like it should.
>  > 
>  > Try using "s" instead. This works since ever. I didn't even know,
>  > that the other option exists, too.
> 
> That's a workaround for this particular case, but the name-munging
> is still wrong and broken.
> 
> With "foo-bar=fie-fum" passed to the kernel, "foo_bar=fie-fum" is
> what's put into init's environment. (I checked.)

 $ foo-bar=fie-fum
  bash: foo-bar=fie-fum: command not found

 $ foo_bar=fie-fum

I just mean to say, it is not exactly usual names one
get to see in an ordinary environment.

Still, i think you are right, it is somewhat unexpected.

-alex

Re: kernel param and KBUILD_MODNAME name-munging mess

[Kai Germaschewski]

On Wed, 22 Jan 2003, Mikael Pettersson wrote:

> Ingo Oeser writes:
>  > On Mon, Jan 20, 2003 at 02:41:03PM +0100, Mikael Pettersson wrote:
>  > > Booting kernel 2.5.59 with the "-s" kernel boot parameter
>  > > doesn't get you into single-user mode like it should.
>  > 
>  > Try using "s" instead. This works since ever. I didn't even know,
>  > that the other option exists, too.
> 
> That's a workaround for this particular case, but the name-munging
> is still wrong and broken.
> 
> With "foo-bar=fie-fum" passed to the kernel, "foo_bar=fie-fum" is
> what's put into init's environment. (I checked.)

I agree that the current behavior is unexpected and probably should be 
fixed. There's basically two ways:
o Pass KBUILD_MODNAME as a string without munging
o Change the setup code to not alter the command line (either use a
  special version of strcmp which knows about "-,_", or work on a 
  temporary copy)

KBUILD_BASENAME needs to be an un-stringified symbol following 
certain conventions to make it possible to use it e.g. in 
include/linux/spinlock.h, that's why '-' and ',' are escaped to '_'.

However, for all I can tell, this is not true for KBUILD_MODNAME, since
that seems to be only used for constructing an actual string, which of 
course can contain all kinds of characters. So I think using the first 
approach would be somewhat nicer, as it gets rid of the unintuitive 
"ide-cd" -> "ide_cd" munging on the kernel command line.

Just needs to be done, of course ;)

--Kai

Re: kernel param and KBUILD_MODNAME name-munging mess

[Roman Zippel]

Hi,

Kai Germaschewski wrote:

> KBUILD_BASENAME needs to be an un-stringified symbol following
> certain conventions to make it possible to use it e.g. in
> include/linux/spinlock.h, that's why '-' and ',' are escaped to '_'.
> 
> However, for all I can tell, this is not true for KBUILD_MODNAME, since
> that seems to be only used for constructing an actual string, which of
> course can contain all kinds of characters. So I think using the first
> approach would be somewhat nicer, as it gets rid of the unintuitive
> "ide-cd" -> "ide_cd" munging on the kernel command line.

It might be useful later to build a list of builtin modules. Currently
we do nothing if a builtin module fails to initialize, but we should
disable dependent modules and remove the exported symbols.
Maybe it's better to provide the module name both as label and string.

bye, Roman

Re: kernel param and KBUILD_MODNAME name-munging mess

[Rusty Russell]

In message <15918.28753.632988.981832@harpo.it.uu.se> you write:
> That's a workaround for this particular case, but the name-munging
> is still wrong and broken.

Absolutely agreed.  Patch re-xmitted below.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Avoid mangling - in parameters
Author: Rusty Russell
Status: Trivial (tested in userspace framework)

D: Mikael Pettersson points out that "-s" gets mangled to "_s" on the
D: kernel command line, even though it turns out not to be a
D: parameter.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.59/kernel/params.c working-2.5.59-underscore/kernel/params.c
--- linux-2.5.59/kernel/params.c	2003-01-02 14:48:01.000000000 +1100
+++ working-2.5.59-underscore/kernel/params.c	2003-01-21 18:16:05.000000000 +1100
@@ -27,6 +27,22 @@
 #define DEBUGP(fmt, a...)
 #endif
 
+static inline int dash2underscore(char c)
+{
+	if (c == '-')
+		return '_';
+	return c;
+}
+
+static inline int parameq(const char *input, const char *paramname)
+{
+	unsigned int i;
+	for (i = 0; dash2underscore(input[i]) == paramname[i]; i++)
+		if (input[i] == '\0')
+			return 1;
+	return 0;
+}
+
 static int parse_one(char *param,
 		     char *val,
 		     struct kernel_param *params, 
@@ -37,7 +53,7 @@ static int parse_one(char *param,
 
 	/* Find parameter */
 	for (i = 0; i < num_params; i++) {
-		if (strcmp(param, params[i].name) == 0) {
+		if (parameq(param, params[i].name)) {
 			DEBUGP("They are equal!  Calling %p\n",
 			       params[i].set);
 			return params[i].set(val, &params[i]);
@@ -69,8 +85,6 @@ static char *next_arg(char *args, char *
 		if (equals == 0) {
 			if (args[i] == '=')
 				equals = i;
-			else if (args[i] == '-')
-				args[i] = '_';
 		}
 		if (args[i] == '"')
 			in_quote = !in_quote;