params: make dashes and underscores in parameter names truly equal

PATCH In review Michal Schmidt 3 participants 6 messages Oct 9, 2011 – Oct 19, 2011 Module support

lore

Share & tools

lore

Michal Schmidt

author

2011-10-09 22:03 UTC #0 lore raw

PATCH 1/1 params: make dashes and underscores in parameter names truly equal

The user may use "foo-bar" for a kernel parameter defined as "foo_bar". Make sure it works the other way around too.

Apply the equality of dashes and underscores on early_params and __setup params as well.

The example given in Documentation/kernel-parameters.txt indicates that this is the intended behaviour.

With the patch the kernel accepts "log-buf-len=1M" as expected.
https://bugzilla.redhat.com/show_bug.cgi?id=744545

Signed-off-by Michal Schmidt <[email protected]>

---
 include/linux/moduleparam.h |   20 ++++++++++++++++++++
 init/main.c                 |    4 ++--
 kernel/params.c             |   34 +++++++++++++++++++++++++++-------
 3 files changed, 49 insertions(+), 9 deletions(-)

include/linux/moduleparam.h +20 -0

index 56bda92..609f89c 100644
@@ -267,6 +267,26 @@ static inline void __kernel_param_unlock(void)
 			    .str = &__param_string_##name, 0, perm);	\
 	__MODULE_PARM_TYPE(name, "string")
 
+/**
+ * parameq - checks if two parameter names match
+ * @name1: parameter name 1
+ * @name2: parameter name 2
+ *
+ * Returns true if the two parameter names are equal.
+ * Dashes (-) are considered equal to underscores (_).
+ */
+extern bool parameq(const char *name1, const char *name2);
+
+/**
+ * parameqn - checks if two parameter names match
+ * @name1: parameter name 1
+ * @name2: parameter name 2
+ * @n: the maximum length to compare
+ *
+ * Similar to parameq(), except it compares at most @n characters.
+ */
+extern bool parameqn(const char *name1, const char *name2, size_t n);
+
 /* Called on module insert or kernel boot */
 extern int parse_args(const char *name,
 		      char *args,

init/main.c +2 -2

index 03b408d..63f5f6f 100644
@@ -163,7 +163,7 @@ static int __init obsolete_checksetup(char *line)
 	p = __setup_start;
 	do {
 		int n = strlen(p->str);
-		if (!strncmp(line, p->str, n)) {
+		if (parameqn(line, p->str, n)) {
 			if (p->early) {
 				/* Already done in parse_early_param?
 				 * (Needs exact match on param part).

@@ -392,7 +392,7 @@ static int __init do_early_param(char *param, char *val)
 	const struct obs_kernel_param *p;
 
 	for (p = __setup_start; p < __setup_end; p++) {
-		if ((p->early && strcmp(param, p->str) == 0) ||
+		if ((p->early && parameq(param, p->str)) ||
 		    (strcmp(param, "console") == 0 &&
 		     strcmp(p->str, "earlycon") == 0)
 		) {

kernel/params.c +27 -7

index 1d1c01b..aa8933c 100644
@@ -67,20 +67,40 @@ static void maybe_kfree_parameter(void *param)
 	}
 }
 
-static inline char dash2underscore(char c)
+static char dash2underscore(char c)
 {
 	if (c == '-')
 		return '_';
 	return c;
 }
 
-static inline int parameq(const char *input, const char *paramname)
+bool parameq(const char *name1, const char *name2)
 {
-	unsigned int i;
-	for (i = 0; dash2underscore(input[i]) == paramname[i]; i++)
-		if (input[i] == '\0')
-			return 1;
-	return 0;
+	char c1, c2;
+	while (1) {
+		c1 = dash2underscore(*name1++);
+		c2 = dash2underscore(*name2++);
+		if (c1 != c2)
+			return false;
+		if (c1 == '\0')
+			break;
+	}
+	return true;
+}
+
+bool parameqn(const char *name1, const char *name2, size_t n)
+{
+	char c1, c2;
+	while (n) {
+		c1 = dash2underscore(*name1++);
+		c2 = dash2underscore(*name2++);
+		if (c1 != c2)
+			return false;
+		if (c1 == '\0')
+			break;
+		n--;
+	}
+	return true;
 }
 
 static int parse_one(char *param,

Signature · 1 line

1.7.4.4

Rusty Russell

2011-10-13 00:50 UTC #1 lore raw

Quoted 2 lines · Michal Schmidt

The user may use "foo-bar" for a kernel parameter defined as "foo_bar". Make sure it works the other way around too.

Hi Michal,

Idea is solid, implementation has a few quirks. As the name
obsolete_checksetup() implies, this was only supposed to be a crutch for old __setup() calls. Modern code should be using module_param() or core_param().

Did you have a specific example? Perhaps we should work on converting them all, which would have beneficial side-effects as we poke into old code...

Thanks!
Rusty.

Michal Schmidt

author

2011-10-13 14:41 UTC #2 lore raw

Quoted 4 lines · Rusty Russell

Idea is solid, implementation has a few quirks. As the name
obsolete_checksetup() implies, this was only supposed to be a crutch for old __setup() calls. Modern code should be using module_param() or core_param().

... instead of __setup()?
No objection to the change in do_early_param() ?

Quoted 1 line

Did you have a specific example?

The specific example I had in mind was the one from Documentation/kernel-parameters.txt:

log_buf_len=1M print-fatal-signals=1
can also be entered as
log-buf-len=1M print_fatal_signals=1

'log-buf-len=1M' does not work because it's an early_param(). 'print_fatal_signals=1' does not work because it's defined as __setup("print-fatal-signals=", ...);

Quoted 3 lines

Perhaps we should work on converting
them all, which would have beneficial side-effects as we poke into old code...

Let's see how many __setup() parameters there are:

$ git grep -E '^__setup\("' |wc -l
457

Only the ones with a dash or underscore in the name:

$ git grep -E '__setup\(".*[_-].*"' | wc -l
180

It will take some time.

Thanks,
Michal

Rusty Russell

2011-10-18 06:51 UTC #3 lore raw

Quoted 7 lines · Michal Schmidt

Quoted 4 lines · Rusty Russell
Idea is solid, implementation has a few quirks. As the name
obsolete_checksetup() implies, this was only supposed to be a crutch for old __setup() calls. Modern code should be using module_param() or core_param().
... instead of __setup()?

Yep.

Quoted 1 line

No objection to the change in do_early_param() ?

Well, for the moment I've applied that, with parameq and parameqn reimplemented more cleanly. See below.

Quoted 8 lines

Quoted 1 line
Did you have a specific example?
The specific example I had in mind was the one from Documentation/kernel-parameters.txt:
log_buf_len=1M print-fatal-signals=1
can also be entered as
log-buf-len=1M print_fatal_signals=1

Oops.

Quoted 9 lines

Let's see how many __setup() parameters there are:
$ git grep -E '^__setup\("' |wc -l
457
Only the ones with a dash or underscore in the name:
$ git grep -E '__setup\(".*[_-].*"' | wc -l
180
It will take some time.

The easy things have already been done ;)

Just do the non-arch ones, first, and see where that gets us.

Thanks!
Rusty.

From: Michal Schmidt <[email protected]>
Subject: params: make dashes and underscores in parameter names truly equal
Date: Mon, 10 Oct 2011 00:03:37 +0200

The user may use "foo-bar" for a kernel parameter defined as "foo_bar". Make sure it works the other way around too.

Apply the equality of dashes and underscores on early_params and __setup params as well.

The example given in Documentation/kernel-parameters.txt indicates that this is the intended behaviour.

With the patch the kernel accepts "log-buf-len=1M" as expected.
https://bugzilla.redhat.com/show_bug.cgi?id=744545

Signed-off-by Michal Schmidt <[email protected]>

Signed-off-by Rusty Russell <[email protected]> (neatened implementations)

---
 include/linux/moduleparam.h |   20 ++++++++++++++++++++
 init/main.c                 |    4 ++--
 kernel/params.c             |   21 ++++++++++++++-------
 3 files changed, 36 insertions(+), 9 deletions(-)

include/linux/moduleparam.h +20 -0

@@ -262,6 +262,26 @@ static inline void __kernel_param_unlock
 			    .str = &__param_string_##name, 0, perm);	\
 	__MODULE_PARM_TYPE(name, "string")
 
+/**
+ * parameq - checks if two parameter names match
+ * @name1: parameter name 1
+ * @name2: parameter name 2
+ *
+ * Returns true if the two parameter names are equal.
+ * Dashes (-) are considered equal to underscores (_).
+ */
+extern bool parameq(const char *name1, const char *name2);
+
+/**
+ * parameqn - checks if two parameter names match
+ * @name1: parameter name 1
+ * @name2: parameter name 2
+ * @n: the length to compare
+ *
+ * Similar to parameq(), except it compares @n characters.
+ */
+extern bool parameqn(const char *name1, const char *name2, size_t n);
+
 /* Called on module insert or kernel boot */
 extern int parse_args(const char *name,
 		      char *args,

init/main.c +2 -2

@@ -163,7 +163,7 @@ static int __init obsolete_checksetup(ch
 	p = __setup_start;
 	do {
 		int n = strlen(p->str);
-		if (!strncmp(line, p->str, n)) {
+		if (parameqn(line, p->str, n)) {
 			if (p->early) {
 				/* Already done in parse_early_param?
 				 * (Needs exact match on param part).

@@ -392,7 +392,7 @@ static int __init do_early_param(char *p
 	const struct obs_kernel_param *p;
 
 	for (p = __setup_start; p < __setup_end; p++) {
-		if ((p->early && strcmp(param, p->str) == 0) ||
+		if ((p->early && parameq(param, p->str)) ||
 		    (strcmp(param, "console") == 0 &&
 		     strcmp(p->str, "earlycon") == 0)
 		) {

kernel/params.c +14 -7

@@ -67,20 +67,27 @@ static void maybe_kfree_parameter(void *
 	}
 }
 
-static inline char dash2underscore(char c)
+static char dash2underscore(char c)
 {
 	if (c == '-')
 		return '_';
 	return c;
 }
 
-static inline int parameq(const char *input, const char *paramname)
+bool parameqn(const char *a, const char *b, size_t n)
 {
-	unsigned int i;
-	for (i = 0; dash2underscore(input[i]) == paramname[i]; i++)
-		if (input[i] == '\0')
-			return 1;
-	return 0;
+	size_t i;
+
+	for (i = 0; i < n; i++) {
+		if (dash2underscore(a[i]) != dash2underscore(b[i]))
+			return false;
+	}
+	return true;
+}
+
+bool parameq(const char *a, const char *b)
+{
+	return parameqn(a, b, strlen(a)+1);
 }
 
 static int parse_one(char *param,

Randy Dunlap

2011-10-18 19:01 UTC #4 lore raw

Quoted 48 lines · Rusty Russell

Quoted 7 lines · Michal Schmidt
Quoted 4 lines · Rusty Russell
Idea is solid, implementation has a few quirks. As the name
obsolete_checksetup() implies, this was only supposed to be a crutch for old __setup() calls. Modern code should be using module_param() or core_param().
... instead of __setup()?
Yep.
Quoted 1 line
No objection to the change in do_early_param() ?
Well, for the moment I've applied that, with parameq and parameqn reimplemented more cleanly. See below.
Quoted 8 lines
Quoted 1 line
Did you have a specific example?
The specific example I had in mind was the one from Documentation/kernel-parameters.txt:
log_buf_len=1M print-fatal-signals=1
can also be entered as
log-buf-len=1M print_fatal_signals=1
Oops.
Quoted 9 lines
Let's see how many __setup() parameters there are:
$ git grep -E '^__setup\("' |wc -l
457
Only the ones with a dash or underscore in the name:
$ git grep -E '__setup\(".*[_-].*"' | wc -l
180
It will take some time.
The easy things have already been done ;)
Just do the non-arch ones, first, and see where that gets us.
Thanks!
Rusty.
From: Michal Schmidt <[email protected]>
Subject: params: make dashes and underscores in parameter names truly equal
Date: Mon, 10 Oct 2011 00:03:37 +0200

Quoted 8 lines

The user may use "foo-bar" for a kernel parameter defined as "foo_bar". Make sure it works the other way around too.
Apply the equality of dashes and underscores on early_params and __setup params as well.
The example given in Documentation/kernel-parameters.txt indicates that this is the intended behaviour.

Well, that could have just been my misunderstanding, but I think that this patch is the right thing to do. Thanks.

Quoted 10 lines

With the patch the kernel accepts "log-buf-len=1M" as expected.
https://bugzilla.redhat.com/show_bug.cgi?id=744545
Signed-off-by Michal Schmidt <[email protected]>
Signed-off-by Rusty Russell <[email protected]> (neatened implementations)
---
 include/linux/moduleparam.h |   20 ++++++++++++++++++++
 init/main.c                 |    4 ++--
 kernel/params.c             |   21 ++++++++++++++-------
 3 files changed, 36 insertions(+), 9 deletions(-)

Signature · 2 lines

~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Rusty Russell

2011-10-19 03:05 UTC #5 lore raw

Quoted 2 lines · Randy Dunlap

Well, that could have just been my misunderstanding, but I think that this patch is the right thing to do. Thanks.

It does two things:
1) It fixes early_params to be case-insensitive. Good.
2) It fixes the (obsolete) __setup() to be case-insensitive.

We should finally get rid of __setup, then we wouldn't need the second part.

Cheers,
Rusty.