When trying to migrate to using bootconfig to embed the kernel's and
PID1's command line with the kernel image itself, and so allowing
changing that without modifying the bootloader, I noticed that
/proc/cmdline changed from e.g.
console=ttymxc0,115200n8 cma=128M quiet -- --log-level=notice
to
console="ttymxc0,115200n8" cma="128M" quiet -- --log-level="notice"
The kernel parameters are parsed just fine, and the quotes are indeed
stripped from the actual argv[] given to PID1. However, the quoting
doesn't really serve any purpose and looks excessive, and might
confuse some (naive) userspace tool trying to parse /proc/cmdline. So
do not quote the value unless it contains whitespace.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
init/main.c | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/init/main.c b/init/main.c
index e24b0780fdff..a658c00a0208 100644
--- a/init/main.c
+++ b/init/main.c
@@ -319,12 +319,20 @@ static char xbc_namebuf[XBC_KEYLEN_MAX] __initdata;
#define rest(dst, end) ((end) > (dst) ? (end) - (dst) : 0)
+static int has_space(const char *v)
+{
+ for (; *v; v++)
+ if (isspace(*v))
+ return 1;
+ return 0;
+}
+
static int __init xbc_snprint_cmdline(char *buf, size_t size,
struct xbc_node *root)
{
struct xbc_node *knode, *vnode;
char *end = buf + size;
- const char *val;
+ const char *val, *q;
int ret;
xbc_node_for_each_key_value(root, knode, val) {
@@ -342,8 +350,9 @@ static int __init xbc_snprint_cmdline(char *buf, size_t size,
continue;
}
xbc_array_for_each_value(vnode, val) {
- ret = snprintf(buf, rest(buf, end), "%s=\"%s\" ",
- xbc_namebuf, val);
+ q = has_space(val) ? "\"" : "";
+ ret = snprintf(buf, rest(buf, end), "%s=%s%s%s ",
+ xbc_namebuf, q, val, q);
if (ret < 0)
return ret;
buf += ret;
--
2.40.1.1.g1c60b9335d
On Wed, 6 Mar 2024 13:24:52 +0100 Rasmus Villemoes <[email protected]> wrote:
> When trying to migrate to using bootconfig to embed the kernel's and
> PID1's command line with the kernel image itself, and so allowing
> changing that without modifying the bootloader, I noticed that
> /proc/cmdline changed from e.g.
>
> console=ttymxc0,115200n8 cma=128M quiet -- --log-level=notice
>
> to
>
> console="ttymxc0,115200n8" cma="128M" quiet -- --log-level="notice"
>
> The kernel parameters are parsed just fine, and the quotes are indeed
> stripped from the actual argv[] given to PID1. However, the quoting
> doesn't really serve any purpose and looks excessive, and might
> confuse some (naive) userspace tool trying to parse /proc/cmdline. So
> do not quote the value unless it contains whitespace.
>
> ...
>
> --- a/init/main.c
> +++ b/init/main.c
> @@ -319,12 +319,20 @@ static char xbc_namebuf[XBC_KEYLEN_MAX] __initdata;
>
> #define rest(dst, end) ((end) > (dst) ? (end) - (dst) : 0)
>
> +static int has_space(const char *v)
> +{
> + for (; *v; v++)
> + if (isspace(*v))
> + return 1;
> + return 0;
> +}
Do we already have something which does this?
Could do strchr(' ')||strchr('\t')
Do we really support tab separation here? I doubt if that gets used or
tested much.
This function could be __init.
On 06/03/2024 21.42, Andrew Morton wrote:
> On Wed, 6 Mar 2024 13:24:52 +0100 Rasmus Villemoes <[email protected]> wrote:
>
>> When trying to migrate to using bootconfig to embed the kernel's and
>> PID1's command line with the kernel image itself, and so allowing
>> changing that without modifying the bootloader, I noticed that
>> /proc/cmdline changed from e.g.
>>
>> console=ttymxc0,115200n8 cma=128M quiet -- --log-level=notice
>>
>> to
>>
>> console="ttymxc0,115200n8" cma="128M" quiet -- --log-level="notice"
>>
>> The kernel parameters are parsed just fine, and the quotes are indeed
>> stripped from the actual argv[] given to PID1. However, the quoting
>> doesn't really serve any purpose and looks excessive, and might
>> confuse some (naive) userspace tool trying to parse /proc/cmdline. So
>> do not quote the value unless it contains whitespace.
>>
>> ...
>>
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -319,12 +319,20 @@ static char xbc_namebuf[XBC_KEYLEN_MAX] __initdata;
>>
>> #define rest(dst, end) ((end) > (dst) ? (end) - (dst) : 0)
>>
>> +static int has_space(const char *v)
>> +{
>> + for (; *v; v++)
>> + if (isspace(*v))
>> + return 1;
>> + return 0;
>> +}
>
> Do we already have something which does this?
Well, 'value[strcspn(value, " \t\r\n")] ? "\"" : ""' would be a
oneliner, but not particularly readable. Also that list of characters
doesn't necessarily match isspace(), see below.
> Could do strchr(' ')||strchr('\t')
>
> Do we really support tab separation here? I doubt if that gets used or
> tested much.
Indeed, I did consider just doing strchr(' '), but ended up with
something based on isspace() as that is what next_arg() in lib/cmdline.c
uses, and also what lib/bootconfig.c allows through (together with
isprint() of course). But yes, \t, \r and \n are unlikely to be used,
even more so \f and \v, but perhaps somebody has 0xa0 by accident (nbsp
in latin1) which our isspace() also allows.
I didn't really want to risk a "prettify the 99% normal use cases" patch
breaking some odd existing setup, so went with the full isspace() check
which really wanted to be done in a helper.
> This function could be __init.
True, but the compiler inlines it to its only caller.
Rasmus
On 07/03/2024 09.10, Rasmus Villemoes wrote:
>>> +static int has_space(const char *v)
>>> +{
>>> + for (; *v; v++)
>>> + if (isspace(*v))
>>> + return 1;
>>> + return 0;
>>> +}
>>
>> Do we already have something which does this?
>
> Well, 'value[strcspn(value, " \t\r\n")] ? "\"" : ""' would be a
> oneliner, but not particularly readable. Also that list of characters
> doesn't necessarily match isspace(), see below.
I didn't look close enough. We do have strpbrk(), so strpbrk(value, "
\t\r\n") ? .. : .. , but that still leaves the question of just what set
of characters to search for. But there's no harm in just making it "
\t\n\v\f\r\xa0" except it requires a comment saying "these are precisely
the isspace() characters in the kernel's ctype".
Rasmus
On Thu, 7 Mar 2024 09:18:17 +0100
Rasmus Villemoes <[email protected]> wrote:
> On 07/03/2024 09.10, Rasmus Villemoes wrote:
>
> >>> +static int has_space(const char *v)
> >>> +{
> >>> + for (; *v; v++)
> >>> + if (isspace(*v))
> >>> + return 1;
> >>> + return 0;
> >>> +}
> >>
> >> Do we already have something which does this?
> >
> > Well, 'value[strcspn(value, " \t\r\n")] ? "\"" : ""' would be a
> > oneliner, but not particularly readable. Also that list of characters
> > doesn't necessarily match isspace(), see below.
>
> I didn't look close enough. We do have strpbrk(), so strpbrk(value, "
> \t\r\n") ? .. : .. , but that still leaves the question of just what set
> of characters to search for. But there's no harm in just making it "
> \t\n\v\f\r\xa0" except it requires a comment saying "these are precisely
> the isspace() characters in the kernel's ctype".
I think strpbrk(" \t\r\n") is good enough. Some may not work in cmdline
but as you said, since next_arg()@lib/cmdline.c uses isspace() to separate
the argument, it is better to use the same rule.
Thank you,
>
> Rasmus
>
--
Masami Hiramatsu (Google) <[email protected]>