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

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

[Masami Hiramatsu]

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) <mhiramat@kernel.org>

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

[Masami Hiramatsu]

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: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 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;

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

[Masami Hiramatsu]

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: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 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));
 		}

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

[Masami Hiramatsu]

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: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 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);

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

[Masami Hiramatsu]

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

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 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

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

[Masami Hiramatsu]

On Sat, 13 Jun 2020 00:23:18 +0900
Masami Hiramatsu <mhiramat@kernel.org> 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: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  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

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

[Steven Rostedt]

On Sat, 13 Jun 2020 00:23:18 +0900
Masami Hiramatsu <mhiramat@kernel.org> 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: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  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;

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

[Steven Rostedt]

On Sat, 13 Jun 2020 00:23:35 +0900
Masami Hiramatsu <mhiramat@kernel.org> 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: stable@vger.kernel.org
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> ---
>  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

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

[Joe Perches]

On Mon, 2020-06-15 at 15:11 -0400, Steven Rostedt wrote:
> On Sat, 13 Jun 2020 00:23:18 +0900
> Masami Hiramatsu <mhiramat@kernel.org> 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.

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

[Steven Rostedt]

On Mon, 15 Jun 2020 12:24:00 -0700
Joe Perches <joe@perches.com> 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

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

[Randy Dunlap]

On 6/15/20 2:21 PM, Steven Rostedt wrote:
> On Mon, 15 Jun 2020 12:24:00 -0700
> Joe Perches <joe@perches.com> 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

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

[Steven Rostedt]

On Mon, 15 Jun 2020 15:30:41 -0700
Randy Dunlap <rdunlap@infradead.org> 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

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

[Randy Dunlap]

On 6/15/20 3:42 PM, Steven Rostedt wrote:
> On Mon, 15 Jun 2020 15:30:41 -0700
> Randy Dunlap <rdunlap@infradead.org> 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 <rdunlap@infradead.org>

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

[Joe Perches]

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 <rdunlap@infradead.org> 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/

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

[Masami Hiramatsu]

On Mon, 15 Jun 2020 15:14:42 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 13 Jun 2020 00:23:35 +0900
> Masami Hiramatsu <mhiramat@kernel.org> 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: stable@vger.kernel.org
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  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 <mhiramat@kernel.org>

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

[Masami Hiramatsu]

On Mon, 15 Jun 2020 15:11:39 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Sat, 13 Jun 2020 00:23:18 +0900
> Masami Hiramatsu <mhiramat@kernel.org> 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: stable@vger.kernel.org
> > Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
> > ---
> >  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 <mhiramat@kernel.org>

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

[Greg KH]

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

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

[Markus Elfring]

> 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