2020-06-12 15:25:31

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 0/4] bootconfig: Fix quote-character issue and return value

Hi Steve,

I found 2 bugs in /proc/bootconfig and tools/bootconfig.

- They always use double-quote to quote values. For the values
which includes double-quote, it should use single-quote instead.
- tools/bootconfig always returns error code if it shows the
bootconfig in initrd (executed without options)

This series fixes those bugs and add testcases to ensure
no regressions.

Thank you,

---

Masami Hiramatsu (4):
proc/bootconfig: Fix to use correct quotes for value
tools/bootconfig: Fix to use correct quotes for value
tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig
tools/bootconfig: Add testcase for show-command and quotes test


fs/proc/bootconfig.c | 13 +++++++++----
tools/bootconfig/main.c | 18 +++++++++++-------
tools/bootconfig/test-bootconfig.sh | 10 ++++++++++
3 files changed, 30 insertions(+), 11 deletions(-)

--
Masami Hiramatsu (Linaro) <[email protected]>


2020-06-12 15:25:40

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

Fix /proc/bootconfig to show the correctly choose the
double or single quotes according to the value.

If a bootconfig value includes a double quote character,
we must use single-quotes to quote that value.

Fixes: c1a3c36017d4 ("proc: bootconfig: Add /proc/bootconfig to show boot config list")
Cc: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
---
fs/proc/bootconfig.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
index 9955d75c0585..930d1dae33eb 100644
--- a/fs/proc/bootconfig.c
+++ b/fs/proc/bootconfig.c
@@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
{
struct xbc_node *leaf, *vnode;
const char *val;
+ char q;
char *key, *end = dst + size;
int ret = 0;

@@ -41,16 +42,20 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
break;
dst += ret;
vnode = xbc_node_get_child(leaf);
- if (vnode && xbc_node_is_array(vnode)) {
+ if (vnode) {
xbc_array_for_each_value(vnode, val) {
- ret = snprintf(dst, rest(dst, end), "\"%s\"%s",
- val, vnode->next ? ", " : "\n");
+ if (strchr(val, '"'))
+ q = '\'';
+ else
+ q = '"';
+ ret = snprintf(dst, rest(dst, end), "%c%s%c%s",
+ q, val, q, vnode->next ? ", " : "\n");
if (ret < 0)
goto out;
dst += ret;
}
} else {
- ret = snprintf(dst, rest(dst, end), "\"%s\"\n", val);
+ ret = snprintf(dst, rest(dst, end), "\"\"\n");
if (ret < 0)
break;
dst += ret;

2020-06-12 15:27:47

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 2/4] tools/bootconfig: Fix to use correct quotes for value

Fix bootconfig tool to show the correctly choose the
double or single quotes according to the value.

If a bootconfig value includes a double quote character,
we must use single-quotes to quote that value.

Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
Cc: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/bootconfig/main.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 0efaf45f7367..21896a6675fd 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -14,13 +14,18 @@
#include <linux/kernel.h>
#include <linux/bootconfig.h>

-static int xbc_show_array(struct xbc_node *node)
+static int xbc_show_value(struct xbc_node *node)
{
const char *val;
+ char q;
int i = 0;

xbc_array_for_each_value(node, val) {
- printf("\"%s\"%s", val, node->next ? ", " : ";\n");
+ if (strchr(val, '"'))
+ q = '\'';
+ else
+ q = '"';
+ printf("%c%s%c%s", q, val, q, node->next ? ", " : ";\n");
i++;
}
return i;
@@ -48,10 +53,7 @@ static void xbc_show_compact_tree(void)
continue;
} else if (cnode && xbc_node_is_value(cnode)) {
printf("%s = ", xbc_node_get_data(node));
- if (cnode->next)
- xbc_show_array(cnode);
- else
- printf("\"%s\";\n", xbc_node_get_data(cnode));
+ xbc_show_value(cnode);
} else {
printf("%s;\n", xbc_node_get_data(node));
}

2020-06-12 15:27:57

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig

Fix bootconfig to return 0 if succeeded to show the bootconfig
in initrd. Without this fix, "bootconfig INITRD" command
returns !0 even if the command succeeded to show the bootconfig.

Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
Cc: [email protected]
Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/bootconfig/main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
index 21896a6675fd..ff2cc9520e10 100644
--- a/tools/bootconfig/main.c
+++ b/tools/bootconfig/main.c
@@ -209,8 +209,10 @@ int show_xbc(const char *path)
ret = load_xbc_from_initrd(fd, &buf);
if (ret < 0)
pr_err("Failed to load a boot config from initrd: %d\n", ret);
- else
+ else {
xbc_show_compact_tree();
+ ret = 0;
+ }

close(fd);
free(buf);

2020-06-12 15:28:15

by Masami Hiramatsu

[permalink] [raw]
Subject: [PATCH 4/4] tools/bootconfig: Add testcase for show-command and quotes test

Add testcases for applied bootconfig showing command
and double/single quotes issues.

Signed-off-by: Masami Hiramatsu <[email protected]>
---
tools/bootconfig/test-bootconfig.sh | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index eff16b77d5eb..3c2ab9e75730 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -55,6 +55,9 @@ echo "Apply command test"
xpass $BOOTCONF -a $TEMPCONF $INITRD
new_size=$(stat -c %s $INITRD)

+echo "Show command test"
+xpass $BOOTCONF $INITRD
+
echo "File size check"
xpass test $new_size -eq $(expr $bconf_size + $initrd_size + 9 + 12)

@@ -114,6 +117,13 @@ xpass grep -q "bar" $OUTFILE
xpass grep -q "baz" $OUTFILE
xpass grep -q "qux" $OUTFILE

+echo "Double/single quotes test"
+echo "key = '\"string\"';" > $TEMPCONF
+$BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF $INITRD > $TEMPCONF
+cat $TEMPCONF
+xpass grep \'\"string\"\' $TEMPCONF
+
echo "=== expected failure cases ==="
for i in samples/bad-* ; do
xfail $BOOTCONF -a $i $INITRD

2020-06-12 16:18:57

by Markus Elfring

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

> Fix /proc/bootconfig to show the correctly choose the
> double or single quotes according to the value.

I suggest to improve this wording a bit.

Regards,
Markus

2020-06-12 22:44:44

by Masami Hiramatsu

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Sat, 13 Jun 2020 00:23:18 +0900
Masami Hiramatsu <[email protected]> wrote:

> Fix /proc/bootconfig to show the correctly choose the
> double or single quotes according to the value.

Oops, I missed to remove "show".

Fix /proc/bootconfig to correctly choose the
double or single quotes according to the value.

>
> If a bootconfig value includes a double quote character,
> we must use single-quotes to quote that value.
>
> Fixes: c1a3c36017d4 ("proc: bootconfig: Add /proc/bootconfig to show boot config list")
> Cc: [email protected]
> Signed-off-by: Masami Hiramatsu <[email protected]>
> ---
> fs/proc/bootconfig.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
> index 9955d75c0585..930d1dae33eb 100644
> --- a/fs/proc/bootconfig.c
> +++ b/fs/proc/bootconfig.c
> @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> {
> struct xbc_node *leaf, *vnode;
> const char *val;
> + char q;
> char *key, *end = dst + size;
> int ret = 0;
>
> @@ -41,16 +42,20 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> break;
> dst += ret;
> vnode = xbc_node_get_child(leaf);
> - if (vnode && xbc_node_is_array(vnode)) {
> + if (vnode) {
> xbc_array_for_each_value(vnode, val) {
> - ret = snprintf(dst, rest(dst, end), "\"%s\"%s",
> - val, vnode->next ? ", " : "\n");
> + if (strchr(val, '"'))
> + q = '\'';
> + else
> + q = '"';
> + ret = snprintf(dst, rest(dst, end), "%c%s%c%s",
> + q, val, q, vnode->next ? ", " : "\n");
> if (ret < 0)
> goto out;
> dst += ret;
> }
> } else {
> - ret = snprintf(dst, rest(dst, end), "\"%s\"\n", val);
> + ret = snprintf(dst, rest(dst, end), "\"\"\n");
> if (ret < 0)
> break;
> dst += ret;
>


--
Masami Hiramatsu

2020-06-13 07:00:43

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Fri, Jun 12, 2020 at 06:15:13PM +0200, Markus Elfring wrote:
> > Fix /proc/bootconfig to show the correctly choose the
> > double or single quotes according to the value.
>
> I suggest to improve this wording a bit.
>
> Regards,

Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list. I strongly suggest that you not do this anymore. Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all. The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback. Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot
> Markus

2020-06-15 19:15:57

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Sat, 13 Jun 2020 00:23:18 +0900
Masami Hiramatsu <[email protected]> wrote:

> Fix /proc/bootconfig to show the correctly choose the
> double or single quotes according to the value.
>
> If a bootconfig value includes a double quote character,
> we must use single-quotes to quote that value.
>
> Fixes: c1a3c36017d4 ("proc: bootconfig: Add /proc/bootconfig to show boot config list")
> Cc: [email protected]
> Signed-off-by: Masami Hiramatsu <[email protected]>
> ---
> fs/proc/bootconfig.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
> index 9955d75c0585..930d1dae33eb 100644
> --- a/fs/proc/bootconfig.c
> +++ b/fs/proc/bootconfig.c
> @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> {
> struct xbc_node *leaf, *vnode;
> const char *val;
> + char q;
> char *key, *end = dst + size;
> int ret = 0;

Hmm, shouldn't the above have the upside-down xmas tree format?

struct xbc_node *leaf, *vnode;
char *key, *end = dst + size;
const char *val;
char q;
int ret = 0;


Looks a little better that way. But anyway, more meat below.

>
> @@ -41,16 +42,20 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> break;
> dst += ret;
> vnode = xbc_node_get_child(leaf);
> - if (vnode && xbc_node_is_array(vnode)) {
> + if (vnode) {
> xbc_array_for_each_value(vnode, val) {
> - ret = snprintf(dst, rest(dst, end), "\"%s\"%s",
> - val, vnode->next ? ", " : "\n");

The above is a functional change that is not described in the change
log.

You use to have:

if (vnode && xbc_node_is_array(vnode)) {
xbc_array_for_each_value() {
[..]
}
} else {
[..]
}

And now have:

if (vnode) {
xbc_array_for_each_value() {
[..]
}
} else {
[..]
}

Is "vnode" equivalent to "vnode && xbc_node_is_array(vnode)" ?

Why was this change made? It seems out of scope with the change log?

-- Steve


> + if (strchr(val, '"'))
> + q = '\'';
> + else
> + q = '"';
> + ret = snprintf(dst, rest(dst, end), "%c%s%c%s",
> + q, val, q, vnode->next ? ", " : "\n");
> if (ret < 0)
> goto out;
> dst += ret;
> }
> } else {
> - ret = snprintf(dst, rest(dst, end), "\"%s\"\n", val);
> + ret = snprintf(dst, rest(dst, end), "\"\"\n");
> if (ret < 0)
> break;
> dst += ret;

2020-06-15 19:16:58

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig

On Sat, 13 Jun 2020 00:23:35 +0900
Masami Hiramatsu <[email protected]> wrote:

> Fix bootconfig to return 0 if succeeded to show the bootconfig
> in initrd. Without this fix, "bootconfig INITRD" command
> returns !0 even if the command succeeded to show the bootconfig.
>
> Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
> Cc: [email protected]
> Signed-off-by: Masami Hiramatsu <[email protected]>
> ---
> tools/bootconfig/main.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> index 21896a6675fd..ff2cc9520e10 100644
> --- a/tools/bootconfig/main.c
> +++ b/tools/bootconfig/main.c
> @@ -209,8 +209,10 @@ int show_xbc(const char *path)
> ret = load_xbc_from_initrd(fd, &buf);
> if (ret < 0)
> pr_err("Failed to load a boot config from initrd: %d\n", ret);
> - else
> + else {
> xbc_show_compact_tree();
> + ret = 0;
> + }

Usually for the above, I think goto is cleaner. As it is strange to
have a successful case as the "else" condition. Not to mention, if you
add brackets for one side of the else, it is usually recommended to add
them for the other side.

But in this case I think it would read better to have:


if (ret < 0) {
pr_err(...);
goto out;
}
xbc_show_compact_tree();
ret = 0;
out:

>
> close(fd);
> free(buf);

-- Steve

2020-06-15 19:30:19

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Mon, 2020-06-15 at 15:11 -0400, Steven Rostedt wrote:
> On Sat, 13 Jun 2020 00:23:18 +0900
> Masami Hiramatsu <[email protected]> wrote:
[]
> > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
[]
> > @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> > {
> > struct xbc_node *leaf, *vnode;
> > const char *val;
> > + char q;
> > char *key, *end = dst + size;
> > int ret = 0;
>
> Hmm, shouldn't the above have the upside-down xmas tree format?
>
> struct xbc_node *leaf, *vnode;
> char *key, *end = dst + size;
> const char *val;
> char q;
> int ret = 0;

Please don't infect kernel sources with that style oddity.


2020-06-15 21:25:40

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Mon, 15 Jun 2020 12:24:00 -0700
Joe Perches <[email protected]> wrote:

> > Hmm, shouldn't the above have the upside-down xmas tree format?
> >
> > struct xbc_node *leaf, *vnode;
> > char *key, *end = dst + size;
> > const char *val;
> > char q;
> > int ret = 0;
>
> Please don't infect kernel sources with that style oddity.

What do you mean? It's already "infected" all over the kernel, (has
been for years!) and I kinda like it. It makes reading variables much
easier on the eyes, and as I get older, that means a lot more ;-)

-- Steve

2020-06-15 22:34:58

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On 6/15/20 2:21 PM, Steven Rostedt wrote:
> On Mon, 15 Jun 2020 12:24:00 -0700
> Joe Perches <[email protected]> wrote:
>
>>> Hmm, shouldn't the above have the upside-down xmas tree format?
>>>
>>> struct xbc_node *leaf, *vnode;
>>> char *key, *end = dst + size;
>>> const char *val;
>>> char q;
>>> int ret = 0;
>>
>> Please don't infect kernel sources with that style oddity.
>
> What do you mean? It's already "infected" all over the kernel, (has
> been for years!) and I kinda like it. It makes reading variables much
> easier on the eyes, and as I get older, that means a lot more ;-)

Yeah, there is some infection, more in some places than others,
but I agree with Joe -- it's not needed or wanted by some of us.


--
~Randy

2020-06-15 22:44:53

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Mon, 15 Jun 2020 15:30:41 -0700
Randy Dunlap <[email protected]> wrote:

> >> Please don't infect kernel sources with that style oddity.
> >
> > What do you mean? It's already "infected" all over the kernel, (has
> > been for years!) and I kinda like it. It makes reading variables much
> > easier on the eyes, and as I get older, that means a lot more ;-)
>
> Yeah, there is some infection, more in some places than others,
> but I agree with Joe -- it's not needed or wanted by some of us.

We all have preferences. But for code that I need to review, I prefer
it.

Why would you be bothered by it? Which is easier on the eyes to read
variables?

struct xbc_node *leaf, *vnode;
const char *val;
char q;
char *key, *end = dst + size;
int ret = 0;

or

struct xbc_node *leaf, *vnode;
char *key, *end = dst + size;
const char *val;
char q;
int ret = 0;

?

-- Steve

2020-06-15 23:16:55

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On 6/15/20 3:42 PM, Steven Rostedt wrote:
> On Mon, 15 Jun 2020 15:30:41 -0700
> Randy Dunlap <[email protected]> wrote:
>
>>>> Please don't infect kernel sources with that style oddity.
>>>
>>> What do you mean? It's already "infected" all over the kernel, (has
>>> been for years!) and I kinda like it. It makes reading variables much
>>> easier on the eyes, and as I get older, that means a lot more ;-)
>>
>> Yeah, there is some infection, more in some places than others,
>> but I agree with Joe -- it's not needed or wanted by some of us.
>
> We all have preferences. But for code that I need to review, I prefer
> it.
>
> Why would you be bothered by it? Which is easier on the eyes to read
> variables?

"to read variables"? I mostly read code, not variables.

> struct xbc_node *leaf, *vnode;
> const char *val;
> char q;
> char *key, *end = dst + size;
> int ret = 0;
>
> or
>
> struct xbc_node *leaf, *vnode;
> char *key, *end = dst + size;
> const char *val;
> char q;
> int ret = 0;
>
> ?

But yes, we all have preferences. For data declaration, mine is more like
order of use or some grouping having to do with locality.

cheers.

--
~Randy
Reported-by: Randy Dunlap <[email protected]>

2020-06-16 05:07:39

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Mon, 2020-06-15 at 16:12 -0700, Randy Dunlap wrote:
> On 6/15/20 3:42 PM, Steven Rostedt wrote:
> > On Mon, 15 Jun 2020 15:30:41 -0700
> > Randy Dunlap <[email protected]> wrote:
> >
> > > > > Please don't infect kernel sources with that style oddity.
> > > >
> > > > What do you mean? It's already "infected" all over the kernel, (has
> > > > been for years!)

Not really. For instance:

$ git grep -A6 "^{" fs/proc/*.[ch]

> But yes, we all have preferences. For data declaration, mine is more like
> order of use or some grouping having to do with locality.
>
> cheers.

Mine too.

But a few years ago I submitted this:
https://lore.kernel.org/patchwork/patch/732076/


2020-06-16 07:55:52

by Masami Hiramatsu

[permalink] [raw]
Subject: Re: [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig

On Mon, 15 Jun 2020 15:14:42 -0400
Steven Rostedt <[email protected]> wrote:

> On Sat, 13 Jun 2020 00:23:35 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
> > Fix bootconfig to return 0 if succeeded to show the bootconfig
> > in initrd. Without this fix, "bootconfig INITRD" command
> > returns !0 even if the command succeeded to show the bootconfig.
> >
> > Fixes: 950313ebf79c ("tools: bootconfig: Add bootconfig command")
> > Cc: [email protected]
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > ---
> > tools/bootconfig/main.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/bootconfig/main.c b/tools/bootconfig/main.c
> > index 21896a6675fd..ff2cc9520e10 100644
> > --- a/tools/bootconfig/main.c
> > +++ b/tools/bootconfig/main.c
> > @@ -209,8 +209,10 @@ int show_xbc(const char *path)
> > ret = load_xbc_from_initrd(fd, &buf);
> > if (ret < 0)
> > pr_err("Failed to load a boot config from initrd: %d\n", ret);
> > - else
> > + else {
> > xbc_show_compact_tree();
> > + ret = 0;
> > + }
>
> Usually for the above, I think goto is cleaner. As it is strange to
> have a successful case as the "else" condition. Not to mention, if you
> add brackets for one side of the else, it is usually recommended to add
> them for the other side.
>
> But in this case I think it would read better to have:
>
>
> if (ret < 0) {
> pr_err(...);
> goto out;
> }
> xbc_show_compact_tree();
> ret = 0;
> out:

Agreed. OK, I'll update it.

Thank you!

>
> >
> > close(fd);
> > free(buf);
>
> -- Steve


--
Masami Hiramatsu <[email protected]>

2020-06-16 08:07:32

by Masami Hiramatsu

[permalink] [raw]
Subject: Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value

On Mon, 15 Jun 2020 15:11:39 -0400
Steven Rostedt <[email protected]> wrote:

> On Sat, 13 Jun 2020 00:23:18 +0900
> Masami Hiramatsu <[email protected]> wrote:
>
> > Fix /proc/bootconfig to show the correctly choose the
> > double or single quotes according to the value.
> >
> > If a bootconfig value includes a double quote character,
> > we must use single-quotes to quote that value.
> >
> > Fixes: c1a3c36017d4 ("proc: bootconfig: Add /proc/bootconfig to show boot config list")
> > Cc: [email protected]
> > Signed-off-by: Masami Hiramatsu <[email protected]>
> > ---
> > fs/proc/bootconfig.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/proc/bootconfig.c b/fs/proc/bootconfig.c
> > index 9955d75c0585..930d1dae33eb 100644
> > --- a/fs/proc/bootconfig.c
> > +++ b/fs/proc/bootconfig.c
> > @@ -27,6 +27,7 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> > {
> > struct xbc_node *leaf, *vnode;
> > const char *val;
> > + char q;
> > char *key, *end = dst + size;
> > int ret = 0;
>
> Hmm, shouldn't the above have the upside-down xmas tree format?
>
> struct xbc_node *leaf, *vnode;
> char *key, *end = dst + size;
> const char *val;
> char q;
> int ret = 0;
>
>
> Looks a little better that way. But anyway, more meat below.

OK.

>
> >
> > @@ -41,16 +42,20 @@ static int __init copy_xbc_key_value_list(char *dst, size_t size)
> > break;
> > dst += ret;
> > vnode = xbc_node_get_child(leaf);
> > - if (vnode && xbc_node_is_array(vnode)) {
> > + if (vnode) {
> > xbc_array_for_each_value(vnode, val) {
> > - ret = snprintf(dst, rest(dst, end), "\"%s\"%s",
> > - val, vnode->next ? ", " : "\n");
>
> The above is a functional change that is not described in the change
> log.
>
> You use to have:
>
> if (vnode && xbc_node_is_array(vnode)) {
> xbc_array_for_each_value() {
> [..]
> }
> } else {
> [..]
> }
>
> And now have:
>
> if (vnode) {
> xbc_array_for_each_value() {
> [..]
> }
> } else {
> [..]
> }
>
> Is "vnode" equivalent to "vnode && xbc_node_is_array(vnode)" ?

No, it's not. But actually, the above change is equivalent, because
xbc_array_for_each_value() can handle the vnode has no "next" member.
(the array means just "a list of value node")

Thus,

if (vnode && xbc_node_is_array(vnode)) {
xbc_array_for_each_value(vnode) /* vnode->next != NULL */
...
} else {
snprintf(val); /* val is an empty string if !vnode */
}

is equivalent to

if (vnode) {
xbc_array_for_each_value(vnode) /* vnode->next can be NULL */
...
} else {
snprintf("");
}

>
> Why was this change made? It seems out of scope with the change log?

Because I want to avoid checking double-quote in each value in 2 places.
If we don't change the if() code, we need

if (strchr(val, '"'))
q = '\'';
else
q = '"';

this in 2 places.

Anyway, I'll add it in the patch comment.

Thank you,

>
> -- Steve
>
>
> > + if (strchr(val, '"'))
> > + q = '\'';
> > + else
> > + q = '"';
> > + ret = snprintf(dst, rest(dst, end), "%c%s%c%s",
> > + q, val, q, vnode->next ? ", " : "\n");
> > if (ret < 0)
> > goto out;
> > dst += ret;
> > }
> > } else {
> > - ret = snprintf(dst, rest(dst, end), "\"%s\"\n", val);
> > + ret = snprintf(dst, rest(dst, end), "\"\"\n");
> > if (ret < 0)
> > break;
> > dst += ret;
>


--
Masami Hiramatsu <[email protected]>