| Message ID | 20220208185008.35843-8-sjg@chromium.org |
|---|---|
| State | Accepted |
| Commit | 7e4b66aa8743aca134883d0ebc42535ce43ecf25 |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | binman: rockchip: Migrate from rockchip SPL_FIT_GENERATOR script | expand |
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 <sjg@chromium.org> > --- > > 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'))
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 <sjg@chromium.org> > --- > > 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!
Hi Alper, On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> 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 <sjg@chromium.org> > > --- > > > > 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 testGetBool(self): > > self.assertEqual(True, fdt_util.GetBool(self.node, 'boolval')) > > self.assertEqual(False, fdt_util.GetBool(self.node, 'missing')) Regards, Simon
On 24/02/2022 01:58, Simon Glass wrote: > On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak@gmail.com> 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...
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'))
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 <sjg@chromium.org> --- 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(+)