[PATCH v3 0/2] bootconfig: Syntax updates

[PATCH v3 0/2] bootconfig: Syntax updates

[Masami Hiramatsu]

Hi,

Here is the 3rd version of the bootconfig syntax update
which remains on my queue.

 - [1/2] A new patch for prohibiting re-definition of value.
 - [2/2] Update the value append operator patch on [1/2].

Thank you,

---

Masami Hiramatsu (2):
      bootconfig: Prohibit re-defining value on same key
      bootconfig: Add append value operator support


 Documentation/admin-guide/bootconfig.rst   |   19 ++++++++++++++++++-
 lib/bootconfig.c                           |   26 ++++++++++++++++++--------
 tools/bootconfig/samples/bad-samekey.bconf |    6 ++++++
 tools/bootconfig/test-bootconfig.sh        |   16 ++++++++++++++--
 4 files changed, 56 insertions(+), 11 deletions(-)
 create mode 100644 tools/bootconfig/samples/bad-samekey.bconf

--
Masami Hiramatsu (Linaro) <mhiramat@kernel.org>

[PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key

[Masami Hiramatsu]

Currently, bootconfig adds new value on the existing key
to the tail of an array. But this looks a bit confusing
because admin can rewrite original value in same config
file easily.

This rejects following value re-definition.

  key = value1
  ...
  key = value2

You should rewrite value1 to value2 in this case.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 Documentation/admin-guide/bootconfig.rst   |   11 ++++++++++-
 lib/bootconfig.c                           |   13 ++++++++-----
 tools/bootconfig/samples/bad-samekey.bconf |    6 ++++++
 3 files changed, 24 insertions(+), 6 deletions(-)
 create mode 100644 tools/bootconfig/samples/bad-samekey.bconf

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index dfeffa73dca3..9ee7650b7817 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -62,7 +62,16 @@ Or more shorter, written as following::
 In both styles, same key words are automatically merged when parsing it
 at boot time. So you can append similar trees or key-values.
 
-Note that a sub-key and a value can not co-exist under a parent key.
+Same-key Values
+---------------
+
+It is prohibited that two or more values or arraies share a same-key.
+For example,::
+
+ foo = bar, baz
+ foo = qux  # !ERROR! we can not re-define same key
+
+Also, a sub-key and a value can not co-exist under a parent key.
 For example, following config is NOT allowed.::
 
  foo = value1
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 54ac623ca781..2ef304db31f2 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -581,7 +581,7 @@ static int __init __xbc_parse_keys(char *k)
 static int __init xbc_parse_kv(char **k, char *v)
 {
 	struct xbc_node *prev_parent = last_parent;
-	struct xbc_node *node, *child;
+	struct xbc_node *child;
 	char *next;
 	int c, ret;
 
@@ -590,15 +590,18 @@ static int __init xbc_parse_kv(char **k, char *v)
 		return ret;
 
 	child = xbc_node_get_child(last_parent);
-	if (child && xbc_node_is_key(child))
-		return xbc_parse_error("Value is mixed with subkey", v);
+	if (child) {
+		if (xbc_node_is_key(child))
+			return xbc_parse_error("Value is mixed with subkey", v);
+		else
+			return xbc_parse_error("Value is redefined", v);
+	}
 
 	c = __xbc_parse_value(&v, &next);
 	if (c < 0)
 		return c;
 
-	node = xbc_add_sibling(v, XBC_VALUE);
-	if (!node)
+	if (!xbc_add_sibling(v, XBC_VALUE))
 		return -ENOMEM;
 
 	if (c == ',') {	/* Array */
diff --git a/tools/bootconfig/samples/bad-samekey.bconf b/tools/bootconfig/samples/bad-samekey.bconf
new file mode 100644
index 000000000000..e8d983a4563c
--- /dev/null
+++ b/tools/bootconfig/samples/bad-samekey.bconf
@@ -0,0 +1,6 @@
+# Same key value is not allowed
+key {
+	foo = value
+	bar = value2
+}
+key.foo = value

Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key

[Randy Dunlap]

On 2/21/20 12:13 AM, Masami Hiramatsu wrote:
> Currently, bootconfig adds new value on the existing key
> to the tail of an array. But this looks a bit confusing
> because admin can rewrite original value in same config
> file easily.
> 
> This rejects following value re-definition.
> 
>   key = value1
>   ...
>   key = value2
> 
> You should rewrite value1 to value2 in this case.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>

Hi,
One correction below and then question.

> ---
>  Documentation/admin-guide/bootconfig.rst   |   11 ++++++++++-
>  lib/bootconfig.c                           |   13 ++++++++-----
>  tools/bootconfig/samples/bad-samekey.bconf |    6 ++++++
>  3 files changed, 24 insertions(+), 6 deletions(-)
>  create mode 100644 tools/bootconfig/samples/bad-samekey.bconf
> 
> diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
> index dfeffa73dca3..9ee7650b7817 100644
> --- a/Documentation/admin-guide/bootconfig.rst
> +++ b/Documentation/admin-guide/bootconfig.rst
> @@ -62,7 +62,16 @@ Or more shorter, written as following::
>  In both styles, same key words are automatically merged when parsing it
>  at boot time. So you can append similar trees or key-values.
>  
> -Note that a sub-key and a value can not co-exist under a parent key.
> +Same-key Values
> +---------------
> +
> +It is prohibited that two or more values or arraies share a same-key.

I think (?):                                   arrays

> +For example,::
> +
> + foo = bar, baz
> + foo = qux  # !ERROR! we can not re-define same key
> +
> +Also, a sub-key and a value can not co-exist under a parent key.
>  For example, following config is NOT allowed.::
>  
>   foo = value1


I'm pretty sure that the kernel command line allows someone to use
  key=value1 ... key=value2
and the first setting is just overwritten with value2 (for most "key"s).

Am I wrong?  and is this patch saying that bootconfig won't operate like that?

thanks.
-- 
~Randy

Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key

[Geert Uytterhoeven]

Hi Randy,

On Sat, Feb 22, 2020 at 5:30 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> On 2/21/20 12:13 AM, Masami Hiramatsu wrote:
> > --- a/Documentation/admin-guide/bootconfig.rst
> > +++ b/Documentation/admin-guide/bootconfig.rst
> > @@ -62,7 +62,16 @@ Or more shorter, written as following::
> >  In both styles, same key words are automatically merged when parsing it
> >  at boot time. So you can append similar trees or key-values.
> >
> > -Note that a sub-key and a value can not co-exist under a parent key.
> > +Same-key Values
> > +---------------
> > +
> > +It is prohibited that two or more values or arraies share a same-key.
>
> I think (?):                                   arrays
>
> > +For example,::
> > +
> > + foo = bar, baz
> > + foo = qux  # !ERROR! we can not re-define same key
> > +
> > +Also, a sub-key and a value can not co-exist under a parent key.
> >  For example, following config is NOT allowed.::
> >
> >   foo = value1
>
>
> I'm pretty sure that the kernel command line allows someone to use
>   key=value1 ... key=value2
> and the first setting is just overwritten with value2 (for most "key"s).
>
> Am I wrong?  and is this patch saying that bootconfig won't operate like that?

I think so. Both are retained.
A typical example is "console=ttyS0 console=tty", to have the kernel output
on both the serial and the graphical console.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key

[Masami Hiramatsu]

On Sat, 22 Feb 2020 10:31:17 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Randy,
> 
> On Sat, Feb 22, 2020 at 5:30 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> > On 2/21/20 12:13 AM, Masami Hiramatsu wrote:
> > > --- a/Documentation/admin-guide/bootconfig.rst
> > > +++ b/Documentation/admin-guide/bootconfig.rst
> > > @@ -62,7 +62,16 @@ Or more shorter, written as following::
> > >  In both styles, same key words are automatically merged when parsing it
> > >  at boot time. So you can append similar trees or key-values.
> > >
> > > -Note that a sub-key and a value can not co-exist under a parent key.
> > > +Same-key Values
> > > +---------------
> > > +
> > > +It is prohibited that two or more values or arraies share a same-key.
> >
> > I think (?):                                   arrays
> >
> > > +For example,::
> > > +
> > > + foo = bar, baz
> > > + foo = qux  # !ERROR! we can not re-define same key
> > > +
> > > +Also, a sub-key and a value can not co-exist under a parent key.
> > >  For example, following config is NOT allowed.::
> > >
> > >   foo = value1
> >
> >
> > I'm pretty sure that the kernel command line allows someone to use
> >   key=value1 ... key=value2
> > and the first setting is just overwritten with value2 (for most "key"s).
> >
> > Am I wrong?  and is this patch saying that bootconfig won't operate like that?
> 
> I think so. Both are retained.
> A typical example is "console=ttyS0 console=tty", to have the kernel output
> on both the serial and the graphical console.

Right, it actually depends on how the option is defined and its handler.
If the option is defined with module_param*() macros, those will be 
overwritten.
But if it is defined with __setup() or early_param(), the handler function
will be called repeatedly. Thus, overwrite or append or skip later one
depends on the option handler.

I think the bootconfig is a bit different from legacy command line at
this moment. The legacy command line can be modified by bootloader,
whereas the bootconfig is a single text file which user can update
each value. Of course bootloader will support the bootconfig to append
some key-values in the future.
So I would like to introduce another "overwrite" operator (":=") and
"assign default" operator ("?=") too. With those operators, the
bootloader can just add their own key-value without decoding the
current bootconfig.

Thank you,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key

[Randy Dunlap]

On 2/22/20 6:41 AM, Masami Hiramatsu wrote:
> On Sat, 22 Feb 2020 10:31:17 +0100
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
>> Hi Randy,
>>
>> On Sat, Feb 22, 2020 at 5:30 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>>> On 2/21/20 12:13 AM, Masami Hiramatsu wrote:
>>>> --- a/Documentation/admin-guide/bootconfig.rst
>>>> +++ b/Documentation/admin-guide/bootconfig.rst
>>>> @@ -62,7 +62,16 @@ Or more shorter, written as following::
>>>>  In both styles, same key words are automatically merged when parsing it
>>>>  at boot time. So you can append similar trees or key-values.
>>>>
>>>> -Note that a sub-key and a value can not co-exist under a parent key.
>>>> +Same-key Values
>>>> +---------------
>>>> +
>>>> +It is prohibited that two or more values or arraies share a same-key.
>>>
>>> I think (?):                                   arrays
>>>
>>>> +For example,::
>>>> +
>>>> + foo = bar, baz
>>>> + foo = qux  # !ERROR! we can not re-define same key
>>>> +
>>>> +Also, a sub-key and a value can not co-exist under a parent key.
>>>>  For example, following config is NOT allowed.::
>>>>
>>>>   foo = value1
>>>
>>>
>>> I'm pretty sure that the kernel command line allows someone to use
>>>   key=value1 ... key=value2
>>> and the first setting is just overwritten with value2 (for most "key"s).
>>>
>>> Am I wrong?  and is this patch saying that bootconfig won't operate like that?
>>
>> I think so. Both are retained.
>> A typical example is "console=ttyS0 console=tty", to have the kernel output
>> on both the serial and the graphical console.

Yes, I was aware of that one also.

> Right, it actually depends on how the option is defined and its handler.
> If the option is defined with module_param*() macros, those will be 
> overwritten.
> But if it is defined with __setup() or early_param(), the handler function
> will be called repeatedly. Thus, overwrite or append or skip later one
> depends on the option handler.

OK, thanks for that clarification.

> I think the bootconfig is a bit different from legacy command line at
> this moment. The legacy command line can be modified by bootloader,
> whereas the bootconfig is a single text file which user can update
> each value. Of course bootloader will support the bootconfig to append
> some key-values in the future.
> So I would like to introduce another "overwrite" operator (":=") and
> "assign default" operator ("?=") too. With those operators, the
> bootloader can just add their own key-value without decoding the
> current bootconfig.


-- 
~Randy

[PATCH v3 2/2] bootconfig: Add append value operator support

[Masami Hiramatsu]

Add append value operator "+=" support to bootconfig syntax.
With this operator, user can add new value to the key as
an entry of array instead of overwriting.
For example,

  foo = bar
  ...
  foo += baz

Then the key "foo" has "bar" and "baz" values as an array.

Signed-off-by: Masami Hiramatsu <mhiramat@kernel.org>
---
 V3: update on value re-definition error patch.
---
 Documentation/admin-guide/bootconfig.rst |   10 +++++++++-
 lib/bootconfig.c                         |   15 +++++++++++----
 tools/bootconfig/test-bootconfig.sh      |   16 ++++++++++++++--
 3 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/Documentation/admin-guide/bootconfig.rst b/Documentation/admin-guide/bootconfig.rst
index 9ee7650b7817..82a657096ebc 100644
--- a/Documentation/admin-guide/bootconfig.rst
+++ b/Documentation/admin-guide/bootconfig.rst
@@ -71,7 +71,15 @@ For example,::
  foo = bar, baz
  foo = qux  # !ERROR! we can not re-define same key
 
-Also, a sub-key and a value can not co-exist under a parent key.
+If you want to append the value to existing key as an array member,
+you can use ``+=`` operator. For example::
+
+ foo = bar, baz
+ foo += qux
+
+In this case, the key ``foo`` has ``bar``, ``baz`` and ``qux``.
+
+However, a sub-key and a value can not co-exist under a parent key.
 For example, following config is NOT allowed.::
 
  foo = value1
diff --git a/lib/bootconfig.c b/lib/bootconfig.c
index 2ef304db31f2..ec3ce7fd299f 100644
--- a/lib/bootconfig.c
+++ b/lib/bootconfig.c
@@ -578,7 +578,7 @@ static int __init __xbc_parse_keys(char *k)
 	return __xbc_add_key(k);
 }
 
-static int __init xbc_parse_kv(char **k, char *v)
+static int __init xbc_parse_kv(char **k, char *v, int op)
 {
 	struct xbc_node *prev_parent = last_parent;
 	struct xbc_node *child;
@@ -593,7 +593,7 @@ static int __init xbc_parse_kv(char **k, char *v)
 	if (child) {
 		if (xbc_node_is_key(child))
 			return xbc_parse_error("Value is mixed with subkey", v);
-		else
+		else if (op == '=')
 			return xbc_parse_error("Value is redefined", v);
 	}
 
@@ -774,7 +774,7 @@ int __init xbc_init(char *buf)
 
 	p = buf;
 	do {
-		q = strpbrk(p, "{}=;\n#");
+		q = strpbrk(p, "{}=+;\n#");
 		if (!q) {
 			p = skip_spaces(p);
 			if (*p != '\0')
@@ -785,8 +785,15 @@ int __init xbc_init(char *buf)
 		c = *q;
 		*q++ = '\0';
 		switch (c) {
+		case '+':
+			if (*q++ != '=') {
+				ret = xbc_parse_error("Wrong '+' operator",
+							q - 2);
+				break;
+			}
+			/* Fall through */
 		case '=':
-			ret = xbc_parse_kv(&p, q);
+			ret = xbc_parse_kv(&p, q, c);
 			break;
 		case '{':
 			ret = xbc_open_brace(&p, q);
diff --git a/tools/bootconfig/test-bootconfig.sh b/tools/bootconfig/test-bootconfig.sh
index adafb7c50940..1411f4c3454f 100755
--- a/tools/bootconfig/test-bootconfig.sh
+++ b/tools/bootconfig/test-bootconfig.sh
@@ -9,7 +9,7 @@ TEMPCONF=`mktemp temp-XXXX.bconf`
 NG=0
 
 cleanup() {
-  rm -f $INITRD $TEMPCONF
+  rm -f $INITRD $TEMPCONF $OUTFILE
   exit $NG
 }
 
@@ -71,7 +71,6 @@ printf " \0\0\0 \0\0\0" >> $INITRD
 $BOOTCONF -a $TEMPCONF $INITRD > $OUTFILE 2>&1
 xfail grep -i "failed" $OUTFILE
 xfail grep -i "error" $OUTFILE
-rm $OUTFILE
 
 echo "Max node number check"
 
@@ -96,6 +95,19 @@ truncate -s 32764 $TEMPCONF
 echo "\"" >> $TEMPCONF	# add 2 bytes + terminal ('\"\n\0')
 xpass $BOOTCONF -a $TEMPCONF $INITRD
 
+echo "Adding same-key values"
+cat > $TEMPCONF << EOF
+key = bar, baz
+key += qux
+EOF
+echo > $INITRD
+
+xpass $BOOTCONF -a $TEMPCONF $INITRD
+$BOOTCONF $INITRD > $OUTFILE
+xpass grep -q "bar" $OUTFILE
+xpass grep -q "baz" $OUTFILE
+xpass grep -q "qux" $OUTFILE
+
 echo "=== expected failure cases ==="
 for i in samples/bad-* ; do
   xfail $BOOTCONF -a $i $INITRD