| Message ID | 28da60231eb848981e858fa33e3b7e33f8547111.1505494668.git.msuchanek@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/6] lib/cmdline.c: Add backslash support to kernel commandline parsing. | expand |
On Fri, Sep 15, 2017 at 07:02:46PM +0200, Michal Suchanek wrote: > for (i = 0; args[i]; i++) { > - if (isspace(args[i]) && !in_quote) > + if (isspace(args[i]) && !in_quote && !backslash) > break; > - if (equals == 0) { > - if (args[i] == '=') > - equals = i; > + > + if ((equals == 0) && (args[i] == '=')) > + equals = i; > + > + if (!backslash) { > + if ((args[i] == '"') || (args[i] == '\\')) { > + if (args[i] == '"') > + in_quote = !in_quote; > + if (args[i] == '\\') > + backslash = 1; > + > + memmove(args + 1, args, i); > + args++; > + i--; > + } > + } else { > + backslash = 0; > } > - if (args[i] == '"') > - in_quote = !in_quote; > } ... and that makes for Unidiomatic Work With Strings Award for this September. Using memmove() for string rewrite is almost always bad taste; in this case it's also (as usual) broken.
On Fri, 15 Sep 2017 18:14:09 +0100 Al Viro <viro@ZenIV.linux.org.uk> wrote: > On Fri, Sep 15, 2017 at 07:02:46PM +0200, Michal Suchanek wrote: > > > for (i = 0; args[i]; i++) { > > - if (isspace(args[i]) && !in_quote) > > + if (isspace(args[i]) && !in_quote && !backslash) > > break; > > - if (equals == 0) { > > - if (args[i] == '=') > > - equals = i; > > + > > + if ((equals == 0) && (args[i] == '=')) > > + equals = i; > > + > > + if (!backslash) { > > + if ((args[i] == '"') || (args[i] == '\\')) > > { > > + if (args[i] == '"') > > + in_quote = !in_quote; > > + if (args[i] == '\\') > > + backslash = 1; > > + > > + memmove(args + 1, args, i); > > + args++; > > + i--; > > + } > > + } else { > > + backslash = 0; > > } > > - if (args[i] == '"') > > - in_quote = !in_quote; > > } > > ... and that makes for Unidiomatic Work With Strings Award for this > September. Using memmove() for string rewrite is almost always bad > taste; in this case it's also (as usual) broken. Care to share how it is broken? Thanks Michal
On Fri, 15 Sep 2017 19:28:56 +0200 Michal Suchánek <msuchanek@suse.de> wrote: > On Fri, 15 Sep 2017 18:14:09 +0100 > Al Viro <viro@ZenIV.linux.org.uk> wrote: > > > On Fri, Sep 15, 2017 at 07:02:46PM +0200, Michal Suchanek wrote: > > > > > for (i = 0; args[i]; i++) { > > > - if (isspace(args[i]) && !in_quote) > > > + if (isspace(args[i]) && !in_quote && !backslash) > > > break; > > > - if (equals == 0) { > > > - if (args[i] == '=') > > > - equals = i; > > > + > > > + if ((equals == 0) && (args[i] == '=')) > > > + equals = i; > > > + > > > + if (!backslash) { > > > + if ((args[i] == '"') || (args[i] == > > > '\\')) { > > > + if (args[i] == '"') > > > + in_quote = !in_quote; > > > + if (args[i] == '\\') > > > + backslash = 1; > > > + > > > + memmove(args + 1, args, i); > > > + args++; > > > + i--; > > > + } > > > + } else { > > > + backslash = 0; > > > } > > > - if (args[i] == '"') > > > - in_quote = !in_quote; > > > } > > > > ... and that makes for Unidiomatic Work With Strings Award for this > > September. Using memmove() for string rewrite is almost always bad > > taste; in this case it's also (as usual) broken. > > Care to share how it is broken? Guess not. I will assume it is perfectly fine then. It works perfectly fine in my testing. Using memmove for string rewrite is not a matter of taste. It is the only library function with sane semantics for rewrite of anything. Then again open-coding it is always an option and maybe in better taste for some :-> Michal
diff --git a/lib/cmdline.c b/lib/cmdline.c index 6d398a8b63fc..d98bdc017545 100644 --- a/lib/cmdline.c +++ b/lib/cmdline.c @@ -193,30 +193,36 @@ bool parse_option_str(const char *str, const char *option) /* * Parse a string to get a param value pair. - * You can use " around spaces, but can't escape ". + * You can use " around spaces, and you can escape with \ * Hyphens and underscores equivalent in parameter names. */ char *next_arg(char *args, char **param, char **val) { unsigned int i, equals = 0; - int in_quote = 0, quoted = 0; + int in_quote = 0, backslash = 0; char *next; - if (*args == '"') { - args++; - in_quote = 1; - quoted = 1; - } - for (i = 0; args[i]; i++) { - if (isspace(args[i]) && !in_quote) + if (isspace(args[i]) && !in_quote && !backslash) break; - if (equals == 0) { - if (args[i] == '=') - equals = i; + + if ((equals == 0) && (args[i] == '=')) + equals = i; + + if (!backslash) { + if ((args[i] == '"') || (args[i] == '\\')) { + if (args[i] == '"') + in_quote = !in_quote; + if (args[i] == '\\') + backslash = 1; + + memmove(args + 1, args, i); + args++; + i--; + } + } else { + backslash = 0; } - if (args[i] == '"') - in_quote = !in_quote; } *param = args; @@ -225,13 +231,6 @@ char *next_arg(char *args, char **param, char **val) else { args[equals] = '\0'; *val = args + equals + 1; - - /* Don't include quotes in value. */ - if ((args[i-1] == '"') && ((quoted) || (**val == '"'))) { - args[i-1] = '\0'; - if (!quoted) - (*val)++; - } } if (args[i]) {
This allows passing quotes in kernel arguments. It is useful for passing fadump nested arguemnts in fadump_extra_args and might be useful if somebody wanted to pass a double quote directly as part of an argument. It is also useful to have quoting grammar more similar to shells and bootloaders. Signed-off-by: Michal Suchanek <msuchanek@suse.de> --- lib/cmdline.c | 41 ++++++++++++++++++++--------------------- 1 file changed, 20 insertions(+), 21 deletions(-)