LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 0/2] bootconfig: Syntax updates
@ 2020-02-21  8:13 Masami Hiramatsu
  2020-02-21  8:13 ` [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key Masami Hiramatsu
  2020-02-21  8:13 ` [PATCH v3 2/2] bootconfig: Add append value operator support Masami Hiramatsu
  0 siblings, 2 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2020-02-21  8:13 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Geert Uytterhoeven, Borislav Petkov, LKML, Ingo Molnar,
	Andrew Morton, Masami Hiramatsu, Peter Zijlstra

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key
  2020-02-21  8:13 [PATCH v3 0/2] bootconfig: Syntax updates Masami Hiramatsu
@ 2020-02-21  8:13 ` Masami Hiramatsu
  2020-02-22  4:20   ` Randy Dunlap
  2020-02-21  8:13 ` [PATCH v3 2/2] bootconfig: Add append value operator support Masami Hiramatsu
  1 sibling, 1 reply; 7+ messages in thread
From: Masami Hiramatsu @ 2020-02-21  8:13 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Geert Uytterhoeven, Borislav Petkov, LKML, Ingo Molnar,
	Andrew Morton, Masami Hiramatsu, Peter Zijlstra

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v3 2/2] bootconfig: Add append value operator support
  2020-02-21  8:13 [PATCH v3 0/2] bootconfig: Syntax updates Masami Hiramatsu
  2020-02-21  8:13 ` [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key Masami Hiramatsu
@ 2020-02-21  8:13 ` Masami Hiramatsu
  1 sibling, 0 replies; 7+ messages in thread
From: Masami Hiramatsu @ 2020-02-21  8:13 UTC (permalink / raw)
  To: Steven Rostedt
  Cc: Geert Uytterhoeven, Borislav Petkov, LKML, Ingo Molnar,
	Andrew Morton, Masami Hiramatsu, Peter Zijlstra

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key
  2020-02-21  8:13 ` [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key Masami Hiramatsu
@ 2020-02-22  4:20   ` Randy Dunlap
  2020-02-22  9:31     ` Geert Uytterhoeven
  0 siblings, 1 reply; 7+ messages in thread
From: Randy Dunlap @ 2020-02-22  4:20 UTC (permalink / raw)
  To: Masami Hiramatsu, Steven Rostedt
  Cc: Geert Uytterhoeven, Borislav Petkov, LKML, Ingo Molnar,
	Andrew Morton, Peter Zijlstra

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key
  2020-02-22  4:20   ` Randy Dunlap
@ 2020-02-22  9:31     ` Geert Uytterhoeven
  2020-02-22 14:41       ` Masami Hiramatsu
  0 siblings, 1 reply; 7+ messages in thread
From: Geert Uytterhoeven @ 2020-02-22  9:31 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: Masami Hiramatsu, Steven Rostedt, Borislav Petkov, LKML,
	Ingo Molnar, Andrew Morton, Peter Zijlstra

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key
  2020-02-22  9:31     ` Geert Uytterhoeven
@ 2020-02-22 14:41       ` Masami Hiramatsu
  2020-02-22 16:24         ` Randy Dunlap
  0 siblings, 1 reply; 7+ messages in thread
From: Masami Hiramatsu @ 2020-02-22 14:41 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Randy Dunlap, Masami Hiramatsu, Steven Rostedt, Borislav Petkov,
	LKML, Ingo Molnar, Andrew Morton, Peter Zijlstra

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key
  2020-02-22 14:41       ` Masami Hiramatsu
@ 2020-02-22 16:24         ` Randy Dunlap
  0 siblings, 0 replies; 7+ messages in thread
From: Randy Dunlap @ 2020-02-22 16:24 UTC (permalink / raw)
  To: Masami Hiramatsu, Geert Uytterhoeven
  Cc: Steven Rostedt, Borislav Petkov, LKML, Ingo Molnar,
	Andrew Morton, Peter Zijlstra

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-02-22 16:24 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-21  8:13 [PATCH v3 0/2] bootconfig: Syntax updates Masami Hiramatsu
2020-02-21  8:13 ` [PATCH v3 1/2] bootconfig: Prohibit re-defining value on same key Masami Hiramatsu
2020-02-22  4:20   ` Randy Dunlap
2020-02-22  9:31     ` Geert Uytterhoeven
2020-02-22 14:41       ` Masami Hiramatsu
2020-02-22 16:24         ` Randy Dunlap
2020-02-21  8:13 ` [PATCH v3 2/2] bootconfig: Add append value operator support Masami Hiramatsu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).