Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
On Mon, 15 Jun 2020 15:11:39 -0400
Steven Rostedt wrote:
> On Sat, 13 Jun 2020 00:23:18 +0900
> Masami Hiramatsu 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
> > ---
> > 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
Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
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 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
On 6/15/20 3:42 PM, Steven Rostedt wrote: > On Mon, 15 Jun 2020 15:30:41 -0700 > Randy Dunlap 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
Re: [PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
On Mon, 15 Jun 2020 15:30:41 -0700 Randy Dunlap 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
On 6/15/20 2:21 PM, Steven Rostedt wrote: > On Mon, 15 Jun 2020 12:24:00 -0700 > Joe Perches 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
On Mon, 15 Jun 2020 12:24:00 -0700 Joe Perches 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
On Mon, 2020-06-15 at 15:11 -0400, Steven Rostedt wrote:
> On Sat, 13 Jun 2020 00:23:18 +0900
> Masami Hiramatsu 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
On Sat, 13 Jun 2020 00:23:18 +0900
Masami Hiramatsu 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
> ---
> 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
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
On Sat, 13 Jun 2020 00:23:18 +0900
Masami Hiramatsu 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: sta...@vger.kernel.org
> Signed-off-by: Masami Hiramatsu
> ---
> 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
> 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
[PATCH 1/4] proc/bootconfig: Fix to use correct quotes for value
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
---
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;
