Hi David,
> On Jun 15, 2016, at 06:14 , David Gibson <da...@gibson.dropbear.id.au> wrote:
>
> On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>>> On Jun 14, 2016, at 03:25 , David Gibson <da...@gibson.dropbear.id.au>
>>> wrote:
>>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> [snip]
>>>>> +static int fdt_overlay_merge(void *dt, void *dto)
>>>>> +{
>>>>> + int root, fragment;
>>>>> +
>>>>> + root = fdt_path_offset(dto, "/");
>>>>> + if (root < 0)
>>>>> + return root;
>>>>> +
>>>>> + fdt_for_each_subnode(dto, fragment, root) {
>>>>> + const char *name = fdt_get_name(dto, fragment, NULL);
>>>>> + uint32_t target;
>>>>> + int overlay;
>>>>> + int ret;
>>>>> +
>>>>> + if (strncmp(name, "fragment", 8))
>>>>> + continue;
>>>>> +
>>>>
>>>> This is incorrect. The use of “fragment” is a convention only.
>>>> The real test whether the node is an overlay fragment is that
>>>> it contains a target property.
>>>
>>> Hmm.. I dislike that approach. First, it means that if new target
>>> types are introduced in future, older code is likely to silently
>>> ignore such fragments instead of realizing that it doesn't know how to
>>> apply themm. Second, it raises weird issues if some node down within
>>> a fragment also happens to have a property called "target”.
>>
>> Not really. If new targets are introduced then the fragment will be ignored.
>
> Yes.. and that's bad.
>
That’s arguable.
>> We can have an argument about what is better to do (report an error or
>> ignore a fragment) but what it comes down to is that that applicator
>> does not know how to handle the new target method.
>
> Sure, let's have the argument. The overlay is constructed on the
> assumption that all the pieces will be applied, or none of them. A
> silent, partial application is an awful, awful failure mode. We
> absolutely should report an error, and in order to do so we need to
> know what are applicable fragments, whether or not we understand the
> target designation (or any other meta-data the fragment has).
>
This way also allows having nodes being something other than fragments.
So instead of being painted into a corner (all subnodes that are not
named ‘fragment@X’ are errors), we have flexibility in introducing
nodes that contain configuration data for the loader.
> Given what's established so far, checking the name seems the obvious
> way to do that.
>
Again, it’s arguable. Stricter checking against future-proofing.
>> There is no issues with any target properties inside a fragment because
>> the check is not made recursively.
>
> Ok, so the real test you're proposing is "at the top level AND has a
> target property”.
Yes
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
Regards
— Pantelis
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
