[PATCH 09/24] dtoc: Support reading a list of arguments

[Simon Glass]

Hi Alper,

On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak at 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 at 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