Re: [PATCH v2 7/9] libfdt: Add overlay application function

[David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]

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-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> 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-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> 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.

> > 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]