Hi David,
> On Jun 15, 2016, at 13:19 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Jun 15, 2016 at 12:34:00PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>>
>>> On Jun 15, 2016, at 06:14 , David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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 <david@xxxxxxxxxxxxxxxxxxxxx> 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.
>
> !?! No, really, silent partial application is just horrible.
>
>>>> 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.
>
> There's no significant difference between the approaches from this
> point of view. Metadata nodes are certainly possible (we already have
> __symbols__ and __fixups__) but calling them something other than
> fragment@ is no harder than leaving off the target property. In fact
> even if it was workable, calling new metadata nodes fragment@ would be
> stupidly confusing.
>
Err, we won’t ever call metadata nodes fragment@, that would be awfully confusing.
We’re bikeshedding, it’s just a philosophical argument right now.
The correct way is to get cracking on the machine readable yaml based bindings
and enforce it through there.
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
