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

[Alper Nebi Yasak]

On 24/02/2022 01:58, Simon Glass wrote:
> On Tue, 15 Feb 2022 at 04:53, Alper Nebi Yasak <alpernebiyasak at 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...