Parsing kernel parameters and escaping "

Parsing kernel parameters and escaping "

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

Hi list,
I was wondering why " cannot be escaped when parsing kernel parameters
in kernel/params.c. Given a disk label like 'foo " bar' and passing
something to the kernel command line like real_root=LABEL="foo \" bar"
this would make 'bar' a kernel parameter which is misleading imho. Any
ideas why this path was chosen and how I am supposed to pass such a
parameter that it is considered as one entity?

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[RFC] Re: Parsing kernel parameters and escaping "

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]

As stated in my last E-Mail in this thread there are currently some
limitations when passing quotes to kernel parameters, I tried to remove
some of those limitations. A git fp is attached.

(from the commit message):
> There was a limitation for kernel parameters with regards to quoting. It
> wasn't possible to escape quotes or use quotes to form space-filled
> values _inside_ parameters. This patch attempts to make that possible,
> kernel parameters are now parsed as follows:
> '"param= value"' [param= value][]
> 'param=" value "" combination "' [param][ value  combination ]
> 'param=" \" test"' [param][ " test]
> '"param"=another' [param][another]

Any suggestions welcome. Please be kind, as I'm not too familar with
writing parsers, kernel development and what I may have broke. :-)

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] Re: Parsing kernel parameters and escaping "

[Daniel Mierswa]

[-- Attachment #1.1: Type: text/plain, Size: 1100 bytes --]

Daniel Mierswa wrote:
> As stated in my last E-Mail in this thread there are currently some
> limitations when passing quotes to kernel parameters, I tried to remove
> some of those limitations. A git fp is attached.
> 
> (from the commit message):
>> There was a limitation for kernel parameters with regards to quoting. It
>> wasn't possible to escape quotes or use quotes to form space-filled
>> values _inside_ parameters. This patch attempts to make that possible,
>> kernel parameters are now parsed as follows:
>> '"param= value"' [param= value][]
>> 'param=" value "" combination "' [param][ value  combination ]
>> 'param=" \" test"' [param][ " test]
>> '"param"=another' [param][another]
> 
> Any suggestions welcome. Please be kind, as I'm not too familar with
> writing parsers, kernel development and what I may have broke. :-)
> 

Great I forgot the patch, here it is now.

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22

[-- Attachment #1.2: kernel.patch --]
[-- Type: text/plain, Size: 4025 bytes --]

From 2dbc124b6c1674592ec5faaae5ca87fc7b16cc12 Mon Sep 17 00:00:00 2001
From: Daniel Mierswa <impulze@impulze.org>
Date: Tue, 7 Jul 2009 00:54:38 +0200
Subject: [PATCH] Attempt to handle quotes in kernel parameters

There was a limitation for kernel parameters with regards to quoting. It
wasn't possible to escape quotes or use quotes to form space-filled
values _inside_ parameters. This patch attempts to make that possible,
kernel parameters are now parsed as follows:
'"param= value"' [param= value][]
'param=" value "" combination "' [param][ value  combination ]
'param=" \" test"' [param][ " test]
'"param"=another' [param][another]
---
 kernel/params.c |  135 ++++++++++++++++++++++++++++++++++++++-----------------
 1 files changed, 93 insertions(+), 42 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index de273ec..0a9fc14 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -75,58 +75,109 @@ static int parse_one(char *param,
 	return -ENOENT;
 }
 
-/* You can use " around spaces, but can't escape ". */
-/* Hyphens and underscores equivalent in parameter names. */
-static char *next_arg(char *args, char **param, char **val)
+/* handle quotes in tokens (parameter and values)
+ * '" foo bar "' => ' foo bar '
+ * '" foo \" "'  => ' foo " '
+ */
+static void add_token(char ** token, char * args)
 {
-	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
-	char *next;
+	char * iterator, * last_quote;
+	int in_quotes;
 
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
+	in_quotes = 0;
+
+	for (iterator = args; *iterator; iterator++) {
+		if (*iterator == '\\' && *(iterator + 1) == '"') {
+			char * mover;
+
+			/* move all characters back */
+			for (mover = iterator; *mover; mover++) {
+				*mover = *(mover + 1);
+			}
+
+			continue;
+		}
+
+		if (*iterator != '"') {
+			continue;
+		}
+
+		if (in_quotes) {
+			char * mover;
+
+			/* move whole string back until current " is reached */
+			for (mover = last_quote; mover != iterator - 1; mover++) {
+				*mover = *(mover + 1);
+			}
+
+			/* ignore the current " and move the rest of the string back */
+			while (*mover) {
+				*mover = *(mover + 2);
+				mover++;
+			}
+
+			/* ignored 2 quotes, decrease the iterator */
+			iterator -= 2;
+			last_quote = NULL;
+		} else {
+			last_quote = iterator;
+		}
+
+		in_quotes = !in_quotes;
 	}
 
-	for (i = 0; args[i]; i++) {
-		if (args[i] == ' ' && !in_quote)
+	*token = args;
+}
+
+static char * next_arg(char * args, char ** param, char ** val)
+{
+	char * token;
+	int in_quotes, is_escaped;
+
+	*param = *val = NULL;
+	in_quotes = is_escaped = 0;
+
+	/* tokenizer */
+	for (token = args; *token; token++) {
+		/* parameter or value is finished */
+		if (!in_quotes && *token == ' ') {
+			*token = '\0';
 			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
 		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
-	}
 
-	*param = args;
-	if (!equals)
-		*val = NULL;
-	else {
-		args[equals] = '\0';
-		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
+		/* parameter/value split */
+		if (!in_quotes && *token == '=' && !*param) {
+			*token = '\0';
+			add_token(param, args);
+			args = token + 1;
+			continue;
+		}
+
+		if (!is_escaped && *token == '\\') {
+			is_escaped = 1;
+			continue;
 		}
-		if (quoted && args[i-1] == '"')
-			args[i-1] = '\0';
+
+		if (*token == '"' && !is_escaped) {
+			in_quotes = !in_quotes;
+		}
+
+		/* always reset escape value, only " needs it */
+		is_escaped = 0;
 	}
 
-	if (args[i]) {
-		args[i] = '\0';
-		next = args + i + 1;
-	} else
-		next = args + i;
+	if (!*param) {
+		add_token(param, args);
+	} else {
+		add_token(val, args);
+	}
+
+	/* remove trailing spaces */
+	while (*token == ' ') {
+		token++;
+	}
 
-	/* Chew up trailing spaces. */
-	while (*next == ' ')
-		next++;
-	return next;
+	return token + 1;
 }
 
 /* Args looks like "foo=bar,bar2 baz=fuz wiz". */
-- 
1.6.3.3


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] Re: Parsing kernel parameters and escaping "

[Daniel Mierswa]

[-- Attachment #1.1: Type: text/plain, Size: 335 bytes --]

Second version of the patch, I accidently forgot to take care of out of
bounds checking when reaching the end of the kernel parameter line.

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22

[-- Attachment #1.2: kernel.patch --]
[-- Type: text/plain, Size: 4265 bytes --]

From fe3bf00a0fc2db941fef41c44fc6d9eefad4d037 Mon Sep 17 00:00:00 2001
From: Daniel Mierswa <impulze@impulze.org>
Date: Tue, 7 Jul 2009 00:54:38 +0200
Subject: [PATCH] Attempt to handle quotes in kernel parameters

There was a limitation for kernel parameters with regards to quoting. It
wasn't possible to escape quotes or use quotes to form space-filled
values _inside_ parameters. This patch attempts to make that possible,
kernel parameters are now parsed as follows:
'"param= value"' [param= value][]
'param=" value "" combination "' [param][ value  combination ]
'param=" \" test"' [param][ " test]
'"param"=another' [param][another]
---
 kernel/params.c |  146 +++++++++++++++++++++++++++++++++++++++----------------
 1 files changed, 104 insertions(+), 42 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index de273ec..73de8f5 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -75,58 +75,120 @@ static int parse_one(char *param,
 	return -ENOENT;
 }
 
-/* You can use " around spaces, but can't escape ". */
-/* Hyphens and underscores equivalent in parameter names. */
-static char *next_arg(char *args, char **param, char **val)
+/* handle quotes in tokens (parameter and values)
+ * '" foo bar "' => ' foo bar '
+ * '" foo \" "'  => ' foo " '
+ */
+static void add_token(char ** token, char * args)
 {
-	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
-	char *next;
+	char * iterator, * last_quote;
+	int in_quotes;
 
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
+	in_quotes = 0;
+	last_quote = NULL;
+
+	for (iterator = args; *iterator; iterator++) {
+		if (*iterator == '\\' && *(iterator + 1) == '"') {
+			char * mover;
+
+			/* move all characters back */
+			for (mover = iterator; *mover; mover++) {
+				*mover = *(mover + 1);
+			}
+
+			continue;
+		}
+
+		if (*iterator != '"') {
+			continue;
+		}
+
+		if (in_quotes) {
+			char * mover;
+
+			/* move whole string back until current " is reached */
+			for (mover = last_quote; mover != iterator - 1; mover++) {
+				*mover = *(mover + 1);
+			}
+
+			/* ignore the current " and move the rest of the string back */
+			while (*mover) {
+				*mover = *(mover + 2);
+				mover++;
+			}
+
+			/* ignored 2 quotes, decrease the iterator */
+			iterator -= 2;
+			last_quote = NULL;
+		} else {
+			last_quote = iterator;
+		}
+
+		in_quotes = !in_quotes;
 	}
 
-	for (i = 0; args[i]; i++) {
-		if (args[i] == ' ' && !in_quote)
+	*token = args;
+}
+
+static char * next_arg(char * args, char ** param, char ** val)
+{
+	char * token, * next;
+	int in_quotes, is_escaped;
+
+	*param = *val = next = NULL;
+	in_quotes = is_escaped = 0;
+
+	/* tokenizer */
+	for (token = args; *token; token++) {
+		/* parameter or value is finished */
+		if (!in_quotes && *token == ' ') {
+			*token = '\0';
+
+			/* there're still characters in the parameter line */
+			next = token + 1;
+
+			/* remove trailing whitespace */
+			while (*next == ' ') {
+				next++;
+			}
+
 			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
 		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
-	}
 
-	*param = args;
-	if (!equals)
-		*val = NULL;
-	else {
-		args[equals] = '\0';
-		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
+		/* parameter/value split */
+		if (!in_quotes && *token == '=' && !*param) {
+			*token = '\0';
+			add_token(param, args);
+			args = token + 1;
+			continue;
 		}
-		if (quoted && args[i-1] == '"')
-			args[i-1] = '\0';
+
+		if (!is_escaped && *token == '\\') {
+			is_escaped = 1;
+			continue;
+		}
+
+		if (*token == '"' && !is_escaped) {
+			in_quotes = !in_quotes;
+		}
+
+		/* always reset escape value, only " needs it */
+		is_escaped = 0;
 	}
 
-	if (args[i]) {
-		args[i] = '\0';
-		next = args + i + 1;
-	} else
-		next = args + i;
+	if (!*param) {
+		add_token(param, args);
+	} else {
+		add_token(val, args);
+	}
+
+	/* there're parameters left in the command line */
+	if (next) {
+		return next;
+	}
 
-	/* Chew up trailing spaces. */
-	while (*next == ' ')
-		next++;
-	return next;
+	/* end of the line */
+	return token;
 }
 
 /* Args looks like "foo=bar,bar2 baz=fuz wiz". */
-- 
1.6.3.3


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC] Re: Parsing kernel parameters and escaping "

[Rusty Russell]

On Tue, 7 Jul 2009 10:24:18 am Daniel Mierswa wrote:
> There was a limitation for kernel parameters with regards to quoting. It
> wasn't possible to escape quotes or use quotes to form space-filled
> values inside parameters.

Hi Daniel!

   Yes, we've never had the ability to escape quotes (and you're the first to
ask), so when I wrote this code I kept it simple.  You can have spaced out
values, but you need to quote the whole thing "param=some value with spaces".

   We have to be careful not to break existing cmdlines tho: I don't know
if anyone uses \ currently, but simply interpreting \" is probably safe.

> +/* handle quotes in tokens (parameter and values)
> + * '" foo bar "' => ' foo bar '
> + * '" foo \" "'  => ' foo " '
> + */
> +static void add_token(char ** token, char * args)

add_token is a weird name for this.  It actually mangles the argument, and it
really should return the char *.

How about something like:
	static unsigned int pull_token(char *args, const char *delim)

Which unescapes and returns the length of the token, or zero if it simply
swallowed delimeters?  Assuming it always nul terminates, then the caller can
simply do:

	while (*args) {
		len = pull_token(args, " \t\n=");
		if (!len)
			/* Leading whitespace. */
			continue;
		*param = args;
		args += len;
		if (args[0] != '=') {
			*val = NULL;
		} else {
			len = pull_token(args+1, " \t\n");
			*val = args;
			args += len;
	}

Important cases to test are:
	x			param = "x", val = NULL
	x=			param = "x", val = ""
	x=y=1		param = "x", val = "y=1"

Plus all variations where x and y contain quotes.

Cheers,
Rusty.

Re: [RFC] Re: Parsing kernel parameters and escaping "

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

Rusty Russell wrote:
>    Yes, we've never had the ability to escape quotes (and you're the first to
> ask), so when I wrote this code I kept it simple.  You can have spaced out
> values, but you need to quote the whole thing "param=some value with spaces".
True, but that would leave quotes where they are instead of removing them.

>    We have to be careful not to break existing cmdlines tho: I don't know
> if anyone uses \ currently, but simply interpreting \" is probably safe.
Yes, the only way that compatibility seems to break is that now \" in a quoted
string is replaced by a single ".

> add_token is a weird name for this.  It actually mangles the argument, and it
> really should return the char *.
Fixed.

> How about something like:
> 	static unsigned int pull_token(char *args, const char *delim)
Fixed.

> Which unescapes and returns the length of the token, or zero if it simply
> swallowed delimeters?  Assuming it always nul terminates, then the caller can
> simply do:
> [...]
Fixed.

> Important cases to test are:
> 	x			param = "x", val = NULL
> 	x=			param = "x", val = ""
> 	x=y=1		param = "x", val = "y=1"
> 
> Plus all variations where x and y contain quotes.
Tested:

|param| => [param[(none)]]
|param=| => [param[]]
|param=value| => [param[value]]
|param=value=withequal | => [param[value=withequal]]
|param="value with spaces" | => [param[value with spaces]]
|param="value with spaces and quotes \"" | => [param[value with spaces and quotes "]]
|param=\"foo\" | => [param[\"foo" ]]
|"param = value" | => [param = value[(none)]]

> Cheers,
> Rusty.

Thanks for your kind feedback, I'm willing to put more effort into this when needed. I really
first wanted to check if patches for this are welcomed.

-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22

[-- Attachment #2: kernel2.patch --]
[-- Type: text/plain, Size: 4158 bytes --]

>From 21402346a295eb539a111b777ee54f3dd5263b6f Mon Sep 17 00:00:00 2001
From: Daniel Mierswa <impulze@impulze.org>
Date: Tue, 7 Jul 2009 00:54:38 +0200
Subject: [PATCH] Attempt to handle quotes in kernel parameters

There was a limitation for kernel parameters with regards to quoting. It
wasn't possible to escape quotes or use quotes to form space-filled
values _inside_ parameters. This patch attempts to make that possible,
kernel parameters are now parsed as follows:
'"param= value"' [param= value][]
'param=" value "" combination "' [param][ value  combination ]
'param=" \" test"' [param][ " test]
'"param"=another' [param][another]

Signed-off-by: Daniel Mierswa <impulze@impulze.org>
---
 kernel/params.c |  128 +++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 82 insertions(+), 46 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 7f6912c..5f9709d 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -72,58 +72,98 @@ static int parse_one(char *param,
 	return -ENOENT;
 }
 
-/* You can use " around spaces, but can't escape ". */
-/* Hyphens and underscores equivalent in parameter names. */
-static char *next_arg(char *args, char **param, char **val)
+/* modifies args with handled quotes
+ * [" foo bar "] => [ foo bar ]
+ * [" foo \" "]  => [ foo " ]
+ * [\"foo]       => [\"foo]
+ * [\"foo\" ]    => [\"foo" ]
+ */
+static size_t pull_token(char *args, char const *delim)
 {
-	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
-	char *next;
+	size_t length = 0;
+	char *iterator = NULL, *last_quote = NULL;
+
+	for (iterator = args; *iterator; iterator++, length++) {
+		if (*iterator == '"') {
+			if (last_quote) {
+				char *mover = last_quote;
+
+				/* move whole string back until current " is reached */
+				while (mover != iterator - 1) {
+					*mover = *(mover + 1);
+					mover++;
+				}
+
+				/* ignore the current " and move the rest of the string back */
+				while (*mover) {
+					*mover = *(mover + 2);
+					mover++;
+				}
+
+				/* ignored 2 quotes, decrease the iterator and length */
+				length -= 2;
+				iterator -= 2;
+				last_quote = NULL;
+			} else {
+				last_quote = iterator;
+			}
 
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
-	}
+			continue;
+		}
 
-	for (i = 0; args[i]; i++) {
-		if (args[i] == ' ' && !in_quote)
-			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
+		if (last_quote) {
+			/* escaped quote */
+			if (*iterator == '\\' && *(iterator + 1) == '"') {
+				char *mover = NULL;
+
+				/* move all characters back */
+				for (mover = iterator; *mover; mover++) {
+					*mover = *(mover + 1);
+				}
+			}
+
+			continue;
+		}
+
+		{
+			/* check for delimiter */
+			char const *delim_iterator = NULL;
+			for (delim_iterator = delim; *delim_iterator; delim_iterator++) {
+				if (*iterator == *delim_iterator) {
+					return length;
+				}
+			}
 		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
 	}
 
+	return length;
+}
+
+static char *next_arg(char *args, char **param, char **val)
+{
+	size_t len;
+
+	/* Chew leading spaces */
+	while (*args == ' ')
+		args++;
+
+	len = pull_token(args, " \t\n=");
 	*param = args;
-	if (!equals)
+	args += len;
+
+	if (*args == '=') {
+		(*param)[len] = '\0';
+		args++;
+		len = pull_token(args, " \t\n");
+		*val = args;
+		args += len;
+		(*val)[len] = '\0';
+	} else {
+		(*param)[len] = '\0';
 		*val = NULL;
-	else {
-		args[equals] = '\0';
-		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
-		}
-		if (quoted && args[i-1] == '"')
-			args[i-1] = '\0';
 	}
 
-	if (args[i]) {
-		args[i] = '\0';
-		next = args + i + 1;
-	} else
-		next = args + i;
-
-	/* Chew up trailing spaces. */
-	while (*next == ' ')
-		next++;
-	return next;
+	return args + 1;
 }
 
 /* Args looks like "foo=bar,bar2 baz=fuz wiz". */
@@ -137,10 +177,6 @@ int parse_args(const char *name,
 
 	DEBUGP("Parsing ARGS: %s\n", args);
 
-	/* Chew leading spaces */
-	while (*args == ' ')
-		args++;
-
 	while (*args) {
 		int ret;
 		int irq_was_disabled;
-- 
1.6.3.3

Re: [RFC] Re: Parsing kernel parameters and escaping "

[Rusty Russell]

On Mon, 13 Jul 2009 03:29:39 am Daniel Mierswa wrote:
> Tested:
> |param| => [param[(none)]]
> |param=| => [param[]]
> |param=value| => [param[value]]
> |param=value=withequal | => [param[value=withequal]]
> |param="value with spaces" | => [param[value with spaces]]
> |param="value with spaces and quotes \"" | => [param[value with spaces and
> | quotes "]] param=\"foo\" | => [param[\"foo" ]]
> |"param = value" | => [param = value[(none)]]

Hi Daniel!

It might be nice to have that test code somewhere at the bottom of param.c,
at least while we're playing with the code.

> Thanks for your kind feedback, I'm willing to put more effort into this
> when needed. I really first wanted to check if patches for this are
> welcomed.

Well, IMO it's a maintainer's job to give feedback, and patches should always be welcomed (even if not applied!).

Now to the details:

> +static size_t pull_token(char *args, char const *delim)
>  {
> -       unsigned int i, equals = 0;
> -       int in_quote = 0, quoted = 0;
> -       char *next;
> +       size_t length = 0;
> +       char *iterator = NULL, *last_quote = NULL;
> +
> +       for (iterator = args; *iterator; iterator++, length++) {

I really prefer "i" instead of "iterator".  I actually think i as an
unsigned/size_t here would probably make the code neater, but that's an aside.

> +               if (*iterator == '"') {
> +                       if (last_quote) {
> +                               char *mover = last_quote;
> +
> +                               /* move whole string back until current " is reached */ 
> +                               while (mover != iterator - 1) {
> +                                       *mover = *(mover + 1);
> +                                       mover++;
> +                               }

memmove?

> +               {
> +                       /* check for delimiter */
> +                       char const *delim_iterator = NULL;
> +                       for (delim_iterator = delim; *delim_iterator; delim_iterator++) {
> +                               if (*iterator == *delim_iterator) {
> +                                       return length;
> +                               }
> +                       }

How about:
	if (strchr(delim, *iterator))
		return length;

> +static char *next_arg(char *args, char **param, char **val)
> +{
> +       size_t len;
> +
> +       /* Chew leading spaces */
> +       while (*args == ' ')
> +               args++;

Note that this will undo another pending patch, which changes this to
isspace() to handle tabs et al.

Thanks!
Rusty.

Re: [RFC] Re: Parsing kernel parameters and escaping "

[Daniel Mierswa]

[-- Attachment #1: Type: text/plain, Size: 1280 bytes --]

Rusty Russell wrote:
> It might be nice to have that test code somewhere at the bottom of param.c,
> at least while we're playing with the code.
Umm, I'm not sure where test-code is supposed to go in kernel code.
Should it be a main() function, a test() function, just a comment, could
you elaborate? All i did now was to build a small program that reads
argv[1] and uses next_arg just like parse_args() in params.c does.

> Well, IMO it's a maintainer's job to give feedback, and patches should always be welcomed (even if not applied!).
Ok, we'll see where this goes. So far I'm fine chatting about this with
everyone.

> I really prefer "i" instead of "iterator".  I actually think i as an
> unsigned/size_t here would probably make the code neater, but that's an aside.
Fixed.

> memmove?
Fixed.

> How about:
> 	if (strchr(delim, *iterator))
> 		return length;
I really should do more C. :-P Fixed.

> Note that this will undo another pending patch, which changes this to
> isspace() to handle tabs et al.
I will re-do the patch against that commit then once it's done.

> Thanks!
Ditto.


-- 
Mierswa, Daniel

If you still don't like it, that's ok: that's why I'm boss. I simply
know better than you do.
               --- Linus Torvalds, comp.os.linux.advocacy, 1996/07/22

[-- Attachment #2: kernel2.patch --]
[-- Type: text/plain, Size: 3797 bytes --]

>From 36d93d40b535ce7f1253d7740f214dc65b2975b9 Mon Sep 17 00:00:00 2001
From: Daniel Mierswa <impulze@impulze.org>
Date: Tue, 7 Jul 2009 00:54:38 +0200
Subject: [PATCH] Attempt to handle quotes in kernel parameters

There was a limitation for kernel parameters with regards to quoting. It
wasn't possible to escape quotes or use quotes to form space-filled
values _inside_ parameters. This patch attempts to make that possible,
kernel parameters are now parsed as follows:
'"param= value"' [param= value][]
'param=" value "" combination "' [param][ value  combination ]
'param=" \" test"' [param][ " test]
'"param"=another' [param][another]

Signed-off-by: Daniel Mierswa <impulze@impulze.org>
---
 kernel/params.c |  111 ++++++++++++++++++++++++++++++++----------------------
 1 files changed, 66 insertions(+), 45 deletions(-)

diff --git a/kernel/params.c b/kernel/params.c
index 7f6912c..5908623 100644
--- a/kernel/params.c
+++ b/kernel/params.c
@@ -72,58 +72,83 @@ static int parse_one(char *param,
 	return -ENOENT;
 }
 
-/* You can use " around spaces, but can't escape ". */
-/* Hyphens and underscores equivalent in parameter names. */
-static char *next_arg(char *args, char **param, char **val)
+/* modifies args with handled quotes
+ * [" foo bar "] => [ foo bar ]
+ * [" foo \" "]  => [ foo " ]
+ * [\"foo]       => [\"foo]
+ * [\"foo\" ]    => [\"foo" ]
+ */
+int printf(char const *,...);
+static size_t pull_token(char *args, char const *delim)
 {
-	unsigned int i, equals = 0;
-	int in_quote = 0, quoted = 0;
-	char *next;
-
-	if (*args == '"') {
-		args++;
-		in_quote = 1;
-		quoted = 1;
-	}
+	size_t i;
+	char *last_quote = NULL;
 
 	for (i = 0; args[i]; i++) {
-		if (args[i] == ' ' && !in_quote)
+		if (args[i] == '"') {
+			if (last_quote) {
+				size_t remain_len = strlen(args + i + 1);
+
+				/* remove both quotes by moving strings */
+				memmove(last_quote, last_quote + 1, args + i - 1 - last_quote);
+				memmove(args + i - 1, args + i + 1, remain_len + 1);
+
+				/* fill the blank we've left */
+				args[i + remain_len] = '\0';
+
+				/* removed 2 quotes, decrease the iterator  */
+				i -= 2;
+				last_quote = NULL;
+			} else {
+				last_quote = args + i;
+			}
+
+			continue;
+		}
+
+		if (last_quote) {
+			/* escaped quote */
+			if (args[i] == '\\' && args[i + 1] == '"') {
+				/* move all characters back */
+				memmove(args + i, args + i + 1, strlen(args + i + 1) + 1);
+			}
+
+			continue;
+		}
+
+		if (strchr(delim, args[i])) {
 			break;
-		if (equals == 0) {
-			if (args[i] == '=')
-				equals = i;
 		}
-		if (args[i] == '"')
-			in_quote = !in_quote;
 	}
 
+	return i;
+}
+
+static char *next_arg(char *args, char **param, char **val)
+{
+	size_t len;
+ 
+	/* Chew leading spaces */
+	while (*args == ' ')
+		args++;
+ 
+	len = pull_token(args, " \t\n=");
 	*param = args;
-	if (!equals)
+	args += len;
+
+	if (*args == '=') {
+		(*param)[len] = '\0';
+		args++;
+		len = pull_token(args, " \t\n");
+		*val = args;
+		args += len;
+		(*val)[len] = '\0';
+	} else {
+		(*param)[len] = '\0';
 		*val = NULL;
-	else {
-		args[equals] = '\0';
-		*val = args + equals + 1;
-
-		/* Don't include quotes in value. */
-		if (**val == '"') {
-			(*val)++;
-			if (args[i-1] == '"')
-				args[i-1] = '\0';
-		}
-		if (quoted && args[i-1] == '"')
-			args[i-1] = '\0';
 	}
 
-	if (args[i]) {
-		args[i] = '\0';
-		next = args + i + 1;
-	} else
-		next = args + i;
-
-	/* Chew up trailing spaces. */
-	while (*next == ' ')
-		next++;
-	return next;
+	return args + 1;
 }
 
 /* Args looks like "foo=bar,bar2 baz=fuz wiz". */
@@ -137,10 +162,6 @@ int parse_args(const char *name,
 
 	DEBUGP("Parsing ARGS: %s\n", args);
 
-	/* Chew leading spaces */
-	while (*args == ' ')
-		args++;
-
 	while (*args) {
 		int ret;
 		int irq_was_disabled;
-- 
1.6.3.3

Re: [RFC] Re: Parsing kernel parameters and escaping "

[Rusty Russell]

On Mon, 13 Jul 2009 12:19:17 pm Daniel Mierswa wrote:
> Rusty Russell wrote:
> > It might be nice to have that test code somewhere at the bottom of
> > param.c, at least while we're playing with the code.
>
> Umm, I'm not sure where test-code is supposed to go in kernel code.
> Should it be a main() function, a test() function, just a comment, could
> you elaborate? All i did now was to build a small program that reads
> argv[1] and uses next_arg just like parse_args() in params.c does.

Usually I write a function like:

#if 0
static int param_result(char *param, char *val)
{
	static int expect;
	const char *params[] = { "foo", "foo", ... };
	const char *vals[] = "bar", NULL, ...};

	BUG_ON(strcmp(param,params[expect]) != 0);
	if (vals[expect] == NULL)
		BUGON(val);
	else
		BUGON(strcmp(val, vals[expect]) != 0);
}

static int test_params(void)
{
	char *str = kstrdup("foo=bar foo ...", GFP_KERNEL);
	parse_args("test", str, NULL, 0, param_result);
	return 0;
}
module_init(test_params);
#endif

> > Thanks!
>
> Ditto.

Thanks for all the work!
Rusty.