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

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

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

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

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