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

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

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

On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:
> Hi David,
> 
> On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:
> > On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:
> > > On 11/07/2016 20:56, Maxime Ripard wrote:
> > [snip]
> > 
> > > > +static int overlay_merge(void *fdt, void *fdto)
> > > > +{
> > > > +	int fragment;
> > > > +
> > > > +	fdt_for_each_subnode(fragment, fdto, 0) {
> > > > +		int overlay;
> > > > +		int target;
> > > > +		int ret;
> > > > +
> > > > +		target = overlay_get_target(fdt, fdto, fragment);
> > > > +		if (target < 0)
> > > > +			continue;
> > > > +
> > > > +		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
> > > > +		if (overlay < 0)
> > > > +			return overlay;
> > 
> > > Why does the absence of a target cause a fragment to be ignored but
> > > the absence of an "__overlay__" property cause the merging to be
> > > abandoned with an error? Can't we just ignore fragments that aren't
> > > recognised?
> > 
> > So, I had the same question.  But fragments we can't make sense MUST
> > cause failures, and not be silently ignored.
> > 
> > An incompletely applied overlay is almost certainly going to cause you
> > horrible grief at some point, so you absolutely want to know early if
> > your overlay is in a format your tool doesn't understand.
> 
> I'm not sure how we can achieve that without applying it once, and see
> if it fails. The obvious things are easy to detect (like a missing
> __overlay__ node), but some others really aren't (like a poorly
> formatted phandle, or one that overflows) without applying it
> entirely. And that seems difficult without malloc.

So, atomically applying either the whole overlay or nothing would be a
nice property, but it is indeed infeasibly difficult to achieve
without malloc().  Well.. we sort of could by making apply_overlay()
take an output buffer separate from the base tree, but that's not what
I'm suggesting.

I'm fine with the base tree being trashed with an incomplete
application when apply_overlay() reports failure.  WHat I'm not ok
with is *silent* failure.  If you ignore fragments you don't
understand, then - if the overlay uses features that aren't supported
by this version of the code - you'll end up with an incompletely
applied overlay while the apply_overlay() function *reports success*.
That is a recipe for disaster.

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