linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] bootconfig: Fix quote-character issue and return value
@ 2020-06-12 15:23 Masami Hiramatsu
  2020-06-12 15:23 ` [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value Masami Hiramatsu
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-12 15:23 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: stable, LKML

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>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-12 15:23 [PATCH 0/4] bootconfig: Fix quote-character issue and return value Masami Hiramatsu
@ 2020-06-12 15:23 ` Masami Hiramatsu
  2020-06-12 22:42   ` Masami Hiramatsu
  2020-06-15 19:11   ` Steven Rostedt
  2020-06-12 15:23 ` [PATCH 2/4] tools/bootconfig: " Masami Hiramatsu
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-12 15:23 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: stable, LKML

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;


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/4] tools/bootconfig: Fix to use correct quotes for value
  2020-06-12 15:23 [PATCH 0/4] bootconfig: Fix quote-character issue and return value Masami Hiramatsu
  2020-06-12 15:23 ` [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value Masami Hiramatsu
@ 2020-06-12 15:23 ` Masami Hiramatsu
  2020-06-12 15:23 ` [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig Masami Hiramatsu
  2020-06-12 15:23 ` [PATCH 4/4] tools/bootconfig: Add testcase for show-command and quotes test Masami Hiramatsu
  3 siblings, 0 replies; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-12 15:23 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: stable, LKML

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));
 		}


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig
  2020-06-12 15:23 [PATCH 0/4] bootconfig: Fix quote-character issue and return value Masami Hiramatsu
  2020-06-12 15:23 ` [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value Masami Hiramatsu
  2020-06-12 15:23 ` [PATCH 2/4] tools/bootconfig: " Masami Hiramatsu
@ 2020-06-12 15:23 ` Masami Hiramatsu
  2020-06-15 19:14   ` Steven Rostedt
  2020-06-12 15:23 ` [PATCH 4/4] tools/bootconfig: Add testcase for show-command and quotes test Masami Hiramatsu
  3 siblings, 1 reply; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-12 15:23 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: stable, LKML

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);


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/4] tools/bootconfig: Add testcase for show-command and quotes test
  2020-06-12 15:23 [PATCH 0/4] bootconfig: Fix quote-character issue and return value Masami Hiramatsu
                   ` (2 preceding siblings ...)
  2020-06-12 15:23 ` [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig Masami Hiramatsu
@ 2020-06-12 15:23 ` Masami Hiramatsu
  3 siblings, 0 replies; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-12 15:23 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: stable, LKML

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-12 15:23 ` [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value Masami Hiramatsu
@ 2020-06-12 22:42   ` Masami Hiramatsu
  2020-06-15 19:11   ` Steven Rostedt
  1 sibling, 0 replies; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-12 22:42 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: Steven Rostedt, stable, LKML, Markus Elfring

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-12 15:23 ` [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value Masami Hiramatsu
  2020-06-12 22:42   ` Masami Hiramatsu
@ 2020-06-15 19:11   ` Steven Rostedt
  2020-06-15 19:24     ` Joe Perches
  2020-06-16  8:05     ` Masami Hiramatsu
  1 sibling, 2 replies; 16+ messages in thread
From: Steven Rostedt @ 2020-06-15 19:11 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: stable, LKML

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;


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig
  2020-06-12 15:23 ` [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig Masami Hiramatsu
@ 2020-06-15 19:14   ` Steven Rostedt
  2020-06-16  7:53     ` Masami Hiramatsu
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2020-06-15 19:14 UTC (permalink / raw)
  To: Masami Hiramatsu; +Cc: stable, LKML

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-15 19:11   ` Steven Rostedt
@ 2020-06-15 19:24     ` Joe Perches
  2020-06-15 21:21       ` Steven Rostedt
  2020-06-16  8:05     ` Masami Hiramatsu
  1 sibling, 1 reply; 16+ messages in thread
From: Joe Perches @ 2020-06-15 19:24 UTC (permalink / raw)
  To: Steven Rostedt, Masami Hiramatsu; +Cc: stable, LKML

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.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-15 19:24     ` Joe Perches
@ 2020-06-15 21:21       ` Steven Rostedt
  2020-06-15 22:30         ` Randy Dunlap
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2020-06-15 21:21 UTC (permalink / raw)
  To: Joe Perches; +Cc: Masami Hiramatsu, stable, LKML

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-15 21:21       ` Steven Rostedt
@ 2020-06-15 22:30         ` Randy Dunlap
  2020-06-15 22:42           ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Randy Dunlap @ 2020-06-15 22:30 UTC (permalink / raw)
  To: Steven Rostedt, Joe Perches; +Cc: Masami Hiramatsu, stable, LKML

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-15 22:30         ` Randy Dunlap
@ 2020-06-15 22:42           ` Steven Rostedt
  2020-06-15 23:12             ` Randy Dunlap
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2020-06-15 22:42 UTC (permalink / raw)
  To: Randy Dunlap; +Cc: Joe Perches, Masami Hiramatsu, stable, LKML

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-15 22:42           ` Steven Rostedt
@ 2020-06-15 23:12             ` Randy Dunlap
  2020-06-16  5:05               ` Joe Perches
  0 siblings, 1 reply; 16+ messages in thread
From: Randy Dunlap @ 2020-06-15 23:12 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Joe Perches, Masami Hiramatsu, stable, LKML

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>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-15 23:12             ` Randy Dunlap
@ 2020-06-16  5:05               ` Joe Perches
  0 siblings, 0 replies; 16+ messages in thread
From: Joe Perches @ 2020-06-16  5:05 UTC (permalink / raw)
  To: Randy Dunlap, Steven Rostedt; +Cc: Masami Hiramatsu, stable, LKML

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/



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig
  2020-06-15 19:14   ` Steven Rostedt
@ 2020-06-16  7:53     ` Masami Hiramatsu
  0 siblings, 0 replies; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-16  7:53 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: stable, LKML

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>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
  2020-06-15 19:11   ` Steven Rostedt
  2020-06-15 19:24     ` Joe Perches
@ 2020-06-16  8:05     ` Masami Hiramatsu
  1 sibling, 0 replies; 16+ messages in thread
From: Masami Hiramatsu @ 2020-06-16  8:05 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: stable, LKML

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>

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-06-16  8:05 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-12 15:23 [PATCH 0/4] bootconfig: Fix quote-character issue and return value Masami Hiramatsu
2020-06-12 15:23 ` [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value Masami Hiramatsu
2020-06-12 22:42   ` Masami Hiramatsu
2020-06-15 19:11   ` Steven Rostedt
2020-06-15 19:24     ` Joe Perches
2020-06-15 21:21       ` Steven Rostedt
2020-06-15 22:30         ` Randy Dunlap
2020-06-15 22:42           ` Steven Rostedt
2020-06-15 23:12             ` Randy Dunlap
2020-06-16  5:05               ` Joe Perches
2020-06-16  8:05     ` Masami Hiramatsu
2020-06-12 15:23 ` [PATCH 2/4] tools/bootconfig: " Masami Hiramatsu
2020-06-12 15:23 ` [PATCH 3/4] tools/bootconfig: Fix to return 0 if succeeded to show the bootconfig Masami Hiramatsu
2020-06-15 19:14   ` Steven Rostedt
2020-06-16  7:53     ` Masami Hiramatsu
2020-06-12 15:23 ` [PATCH 4/4] tools/bootconfig: Add testcase for show-command and quotes test Masami Hiramatsu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).