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

[Masami Hiramatsu <mhiramat@kernel.org>]

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>