Re: [PATCH 09/24] dtoc: Support reading a list of arguments
On 24/02/2022 01:58, Simon Glass wrote:
> On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak
> wrote:
>>> diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
>>> index 19eb13aef3..59e065884f 100644
>>> --- a/tools/dtoc/fdt_util.py
>>> +++ b/tools/dtoc/fdt_util.py
>>> @@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
>>> return [strval]
>>> return value
>>>
>>> +def GetArgs(node, propname):
>>> +prop = node.props.get(propname)
>>> +if not prop:
>>> +raise ValueError(f"Node '{node.path}': Expected property
>>> '{propname}'")
>>> +if prop.bytes:
>>> +value = GetStringList(node, propname)
>>> +else:
>>> +value = []
>>
>> Isn't GetStringList(node, propname, default=[]) enough here, why check
>> prop.bytes?
>
> Because the default value is for when there is no such property. In
> this case there is a property, so the default will not be used.
Ah, it's the same as dict.get(). Don't know why I got confused there.
>>> +lists = [v.split() for v in value]
>>
>> Use shlex.split() to handle quotes inside the strings, so that we can
>> pass args with spaces inside them. e.g. mkimage -n "U-Boot v2022.04".
>> Or each list element could be a single argument with no splitting done.
>>
>> I also wish mkimage -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" from
>> Makefile was possible, but can't think of a great way.
>
> That's actually not what I was trying to do. I want "-n first" to mean
> two args. It is like normally command-line processing.
>
> Of course this means that it isn't possible to do what you want here.
> I am not sure of a good way to support that.
>
> One way would be to only split into args if there is a single string
> in the list? I will try that for now.
That sounds like a reasonable middle ground if you really want to do
splitting.
>>> +args = [x for l in lists for x in l]
>>> +return args
>>> +
>>
>> Anyway, I don't think this belongs here as argument lists are not really
>> a device-tree construct. It would be better in a new binman entry type
>> ("command"?) which mkimage can subclass from.
>
> Well I am trying to keep all typing stuff in here, i.e. everything
> that guesses what is meant by a DT property. That way I can have DT
> tests and expand as needed. I agree this is a borderline case, but I'm
> not sure it is a good to have lots of logic (that depends on the
> internal working of Fdt) in a different file. E.g. I hate all of this
> code and would like to refactor it to put more stuff in pylibfdt one
> day.
That's fair, and I like the idea of refactoring things into pylibfdt.
>>
>>> def GetBool(node, propname, default=False):
>>> """Get an boolean from a property
>>>
>>> diff --git a/tools/dtoc/test/dtoc_test_simple.dts
>>> b/tools/dtoc/test/dtoc_test_simple.dts
>>> index 4c2c70af22..2d321fb034 100644
>>> --- a/tools/dtoc/test/dtoc_test_simple.dts
>>> +++ b/tools/dtoc/test/dtoc_test_simple.dts
>>> @@ -62,5 +62,6 @@
>>>
>>> orig-node {
>>> orig = <1 23 4>;
>>> + args = "-n first", "second", "-p", "123,456", "-x";
>>
>> Could be useful to add an argument with single quotes, and one with
>> escaped double quotes.
>
> I reverted the shlex change in the end...can we do that in a separate
> patch? I think it has interesting implications for the Makefile and we
> should think about what tests to add and what the use cases are.
OK, but I'll need to focus on other things and might forget to ping
later for this...
Re: [PATCH 09/24] dtoc: Support reading a list of arguments
Hi Alper,
On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak wrote:
>
> On 08/02/2022 21:49, Simon Glass wrote:
> > It is helpful to support a string or stringlist containing a list of
> > space-separated arguments, for example:
> >
> >args = "-n fred", "-a", "123";
> >
> > This resolves to the list:
> >
> >-n fred -a 123
>
> Would be clearer as ['-n', 'fred', '-a', '123']
OK
>
> >
> > which can be passed to a program as arguments.
> >
> > Add a helper to do the required processing.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > tools/dtoc/fdt_util.py | 12
> > tools/dtoc/test/dtoc_test_simple.dts | 1 +
> > tools/dtoc/test_fdt.py | 15 +++
> > 3 files changed, 28 insertions(+)
> >
> > diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
> > index 19eb13aef3..59e065884f 100644
> > --- a/tools/dtoc/fdt_util.py
> > +++ b/tools/dtoc/fdt_util.py
> > @@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
> > return [strval]
> > return value
> >
> > +def GetArgs(node, propname):
> > +prop = node.props.get(propname)
> > +if not prop:
> > +raise ValueError(f"Node '{node.path}': Expected property
> > '{propname}'")
> > +if prop.bytes:
> > +value = GetStringList(node, propname)
> > +else:
> > +value = []
>
> Isn't GetStringList(node, propname, default=[]) enough here, why check
> prop.bytes?
Because the default value is for when there is no such property. In
this case there is a property, so the default will not be used.
>
> > +lists = [v.split() for v in value]
>
> Use shlex.split() to handle quotes inside the strings, so that we can
> pass args with spaces inside them. e.g. mkimage -n "U-Boot v2022.04".
> Or each list element could be a single argument with no splitting done.
>
> I also wish mkimage -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" from
> Makefile was possible, but can't think of a great way.
That's actually not what I was trying to do. I want "-n first" to mean
two args. It is like normally command-line processing.
Of course this means that it isn't possible to do what you want here.
I am not sure of a good way to support that.
One way would be to only split into args if there is a single string
in the list? I will try that for now.
>
> > +args = [x for l in lists for x in l]
> > +return args
> > +
>
> Anyway, I don't think this belongs here as argument lists are not really
> a device-tree construct. It would be better in a new binman entry type
> ("command"?) which mkimage can subclass from.
Well I am trying to keep all typing stuff in here, i.e. everything
that guesses what is meant by a DT property. That way I can have DT
tests and expand as needed. I agree this is a borderline case, but I'm
not sure it is a good to have lots of logic (that depends on the
internal working of Fdt) in a different file. E.g. I hate all of this
code and would like to refactor it to put more stuff in pylibfdt one
day.
>
> > def GetBool(node, propname, default=False):
> > """Get an boolean from a property
> >
> > diff --git a/tools/dtoc/test/dtoc_test_simple.dts
> > b/tools/dtoc/test/dtoc_test_simple.dts
> > index 4c2c70af22..2d321fb034 100644
> > --- a/tools/dtoc/test/dtoc_test_simple.dts
> > +++ b/tools/dtoc/test/dtoc_test_simple.dts
> > @@ -62,5 +62,6 @@
> >
> > orig-node {
> > orig = <1 23 4>;
> > + args = "-n first", "second", "-p", "123,456", "-x";
>
> Could be useful to add an argument with single quotes, and one with
> escaped double quotes.
I reverted the shlex change in the end...can we do that in a separate
patch? I think it has interesting implications for the Makefile and we
should think about what tests to add and what the use cases are.
>
> > };
> > };
> > diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
> > index c8fe5fc1de..5d46e69b8b 100755
> > --- a/tools/dtoc/test_fdt.py
> > +++ b/tools/dtoc/test_fdt.py
> > @@ -652,6 +652,21 @@ class TestFdtUtil(unittest.TestCase):
> > self.assertEqual(['test'],
> > fdt_util.GetStringList(self.node, 'missing',
> > ['test']))
> >
> > +def testGetArgs(self):
> > +node = self.dtb.GetNode('/orig-node')
> > +self.assertEqual(['message'], fdt_util.GetArgs(self.node,
> > 'stringval'))
> > +self.assertEqual(
> > +['multi-word', 'message'],
> > +fdt_util.GetArgs(self.node, 'stringarray'))
> > +self.assertEqual([], fdt_util.GetArgs(self.node, 'boolval'))
> > +self.assertEqual(['-n', 'first', 'second', '-p', '123,456', '-x'],
> > + fdt_util.GetArgs(node, 'args'))
> > +with self.assertRaises(ValueError) as exc:
> > +fdt_util.GetArgs(self.node, 'missing')
> > +self.assertIn(
> > +"Node '/spl-test': Expected property 'missing'",
> > +str(exc.exception))
> > +
> > def testGetB
Re: [PATCH 09/24] dtoc: Support reading a list of arguments
On 08/02/2022 21:49, Simon Glass wrote: > It is helpful to support a string or stringlist containing a list of > space-separated arguments, for example: > >args = "-n fred", "-a", "123"; > > This resolves to the list: > >-n fred -a 123 Would be clearer as ['-n', 'fred', '-a', '123'] > > which can be passed to a program as arguments. > > Add a helper to do the required processing. > > Signed-off-by: Simon Glass > --- > > tools/dtoc/fdt_util.py | 12 > tools/dtoc/test/dtoc_test_simple.dts | 1 + > tools/dtoc/test_fdt.py | 15 +++ > 3 files changed, 28 insertions(+) > Applied to u-boot-dm, thanks!
Re: [PATCH 09/24] dtoc: Support reading a list of arguments
On 08/02/2022 21:49, Simon Glass wrote:
> It is helpful to support a string or stringlist containing a list of
> space-separated arguments, for example:
>
>args = "-n fred", "-a", "123";
>
> This resolves to the list:
>
>-n fred -a 123
Would be clearer as ['-n', 'fred', '-a', '123']
>
> which can be passed to a program as arguments.
>
> Add a helper to do the required processing.
>
> Signed-off-by: Simon Glass
> ---
>
> tools/dtoc/fdt_util.py | 12
> tools/dtoc/test/dtoc_test_simple.dts | 1 +
> tools/dtoc/test_fdt.py | 15 +++
> 3 files changed, 28 insertions(+)
>
> diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
> index 19eb13aef3..59e065884f 100644
> --- a/tools/dtoc/fdt_util.py
> +++ b/tools/dtoc/fdt_util.py
> @@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
> return [strval]
> return value
>
> +def GetArgs(node, propname):
> +prop = node.props.get(propname)
> +if not prop:
> +raise ValueError(f"Node '{node.path}': Expected property
> '{propname}'")
> +if prop.bytes:
> +value = GetStringList(node, propname)
> +else:
> +value = []
Isn't GetStringList(node, propname, default=[]) enough here, why check
prop.bytes?
> +lists = [v.split() for v in value]
Use shlex.split() to handle quotes inside the strings, so that we can
pass args with spaces inside them. e.g. mkimage -n "U-Boot v2022.04".
Or each list element could be a single argument with no splitting done.
I also wish mkimage -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" from
Makefile was possible, but can't think of a great way.
> +args = [x for l in lists for x in l]
> +return args
> +
Anyway, I don't think this belongs here as argument lists are not really
a device-tree construct. It would be better in a new binman entry type
("command"?) which mkimage can subclass from.
> def GetBool(node, propname, default=False):
> """Get an boolean from a property
>
> diff --git a/tools/dtoc/test/dtoc_test_simple.dts
> b/tools/dtoc/test/dtoc_test_simple.dts
> index 4c2c70af22..2d321fb034 100644
> --- a/tools/dtoc/test/dtoc_test_simple.dts
> +++ b/tools/dtoc/test/dtoc_test_simple.dts
> @@ -62,5 +62,6 @@
>
> orig-node {
> orig = <1 23 4>;
> + args = "-n first", "second", "-p", "123,456", "-x";
Could be useful to add an argument with single quotes, and one with
escaped double quotes.
> };
> };
> diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
> index c8fe5fc1de..5d46e69b8b 100755
> --- a/tools/dtoc/test_fdt.py
> +++ b/tools/dtoc/test_fdt.py
> @@ -652,6 +652,21 @@ class TestFdtUtil(unittest.TestCase):
> self.assertEqual(['test'],
> fdt_util.GetStringList(self.node, 'missing',
> ['test']))
>
> +def testGetArgs(self):
> +node = self.dtb.GetNode('/orig-node')
> +self.assertEqual(['message'], fdt_util.GetArgs(self.node,
> 'stringval'))
> +self.assertEqual(
> +['multi-word', 'message'],
> +fdt_util.GetArgs(self.node, 'stringarray'))
> +self.assertEqual([], fdt_util.GetArgs(self.node, 'boolval'))
> +self.assertEqual(['-n', 'first', 'second', '-p', '123,456', '-x'],
> + fdt_util.GetArgs(node, 'args'))
> +with self.assertRaises(ValueError) as exc:
> +fdt_util.GetArgs(self.node, 'missing')
> +self.assertIn(
> +"Node '/spl-test': Expected property 'missing'",
> +str(exc.exception))
> +
> def testGetBool(self):
> self.assertEqual(True, fdt_util.GetBool(self.node, 'boolval'))
> self.assertEqual(False, fdt_util.GetBool(self.node, 'missing'))
[PATCH 09/24] dtoc: Support reading a list of arguments
It is helpful to support a string or stringlist containing a list of
space-separated arguments, for example:
args = "-n fred", "-a", "123";
This resolves to the list:
-n fred -a 123
which can be passed to a program as arguments.
Add a helper to do the required processing.
Signed-off-by: Simon Glass
---
tools/dtoc/fdt_util.py | 12
tools/dtoc/test/dtoc_test_simple.dts | 1 +
tools/dtoc/test_fdt.py | 15 +++
3 files changed, 28 insertions(+)
diff --git a/tools/dtoc/fdt_util.py b/tools/dtoc/fdt_util.py
index 19eb13aef3..59e065884f 100644
--- a/tools/dtoc/fdt_util.py
+++ b/tools/dtoc/fdt_util.py
@@ -184,6 +184,18 @@ def GetStringList(node, propname, default=None):
return [strval]
return value
+def GetArgs(node, propname):
+prop = node.props.get(propname)
+if not prop:
+raise ValueError(f"Node '{node.path}': Expected property '{propname}'")
+if prop.bytes:
+value = GetStringList(node, propname)
+else:
+value = []
+lists = [v.split() for v in value]
+args = [x for l in lists for x in l]
+return args
+
def GetBool(node, propname, default=False):
"""Get an boolean from a property
diff --git a/tools/dtoc/test/dtoc_test_simple.dts
b/tools/dtoc/test/dtoc_test_simple.dts
index 4c2c70af22..2d321fb034 100644
--- a/tools/dtoc/test/dtoc_test_simple.dts
+++ b/tools/dtoc/test/dtoc_test_simple.dts
@@ -62,5 +62,6 @@
orig-node {
orig = <1 23 4>;
+ args = "-n first", "second", "-p", "123,456", "-x";
};
};
diff --git a/tools/dtoc/test_fdt.py b/tools/dtoc/test_fdt.py
index c8fe5fc1de..5d46e69b8b 100755
--- a/tools/dtoc/test_fdt.py
+++ b/tools/dtoc/test_fdt.py
@@ -652,6 +652,21 @@ class TestFdtUtil(unittest.TestCase):
self.assertEqual(['test'],
fdt_util.GetStringList(self.node, 'missing',
['test']))
+def testGetArgs(self):
+node = self.dtb.GetNode('/orig-node')
+self.assertEqual(['message'], fdt_util.GetArgs(self.node, 'stringval'))
+self.assertEqual(
+['multi-word', 'message'],
+fdt_util.GetArgs(self.node, 'stringarray'))
+self.assertEqual([], fdt_util.GetArgs(self.node, 'boolval'))
+self.assertEqual(['-n', 'first', 'second', '-p', '123,456', '-x'],
+ fdt_util.GetArgs(node, 'args'))
+with self.assertRaises(ValueError) as exc:
+fdt_util.GetArgs(self.node, 'missing')
+self.assertIn(
+"Node '/spl-test': Expected property 'missing'",
+str(exc.exception))
+
def testGetBool(self):
self.assertEqual(True, fdt_util.GetBool(self.node, 'boolval'))
self.assertEqual(False, fdt_util.GetBool(self.node, 'missing'))
--
2.35.0.263.gb82422642f-goog
