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
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;
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 = '\'';elseq = '"';ret = snprintf(dst, rest(dst, end), "%c%s%c%s",q, val, q, vnode->next ? ", " : "\n"); if (ret < 0) goto out; dst += ret; }
ret = snprintf(dst, rest(dst, end), "\"%s\"\n", val);
ret = snprintf(dst, rest(dst, end), "\"\"\n"); if (ret < 0) break; dst += ret;
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 = '\'';elseq = '"';ret = snprintf(dst, rest(dst, end), "%c%s%c%s",q, val, q, vnode->next ? ", " : "\n"); if (ret < 0) goto out; dst += ret; }
ret = snprintf(dst, rest(dst, end), "\"%s\"\n", val);
ret = snprintf(dst, rest(dst, end), "\"\"\n"); if (ret < 0) break; dst += ret;
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.
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
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.
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
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.
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/
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 = '\'';elseq = '"';ret = snprintf(dst, rest(dst, end), "%c%s%c%s",q, val, q, vnode->next ? ", " : "\n"); if (ret < 0) goto out; dst += ret; }
ret = snprintf(dst, rest(dst, end), "\"%s\"\n", val);
ret = snprintf(dst, rest(dst, end), "\"\"\n"); if (ret < 0) break; dst += ret;
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)); }
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);
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
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
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
linux-stable-mirror@lists.linaro.org
-
Joe Perches -
Masami Hiramatsu -
Masami Hiramatsu -
Randy Dunlap -
Steven Rostedt