On Mon, 15 Jun 2020 15:11:39 -0400
Steven Rostedt <rost...@goodmis.org> wrote:
> On Sat, 13 Jun 2020 00:23:18 +0900
> Masami Hiramatsu <mhira...@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: sta...@vger.kernel.org
> > Signed-off-by: Masami Hiramatsu <mhira...@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 <mhira...@kernel.org>
