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

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

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

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.

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

Given what's established so far, checking the name seems the obvious
way to do that.

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

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