Hi David,
On Jun 15, 2016, at 13:19 , David Gibson david@gibson.dropbear.id.au 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@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 david@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.
!?! 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