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

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

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

On Thu, Jul 14, 2016 at 01:07:45AM +1000, David Gibson wrote:
> 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.

Ok, that makes sense. I'll return an error if the target is missing as
well then.

But then, I think we fall back to the discussion you had with
Pantelis: how do you identify an overlay node (that must have a
target) and some other "metadata" node that shouldn't be applied (and
will not have a target). In the first case, we need to report an error
if it's missing. In the second, we should just ignore the node
entirely.

Would turning that code the other way around, and if it has an
__overlay__ subnode, target or target-path is mandatory, and if not
just ignore the node entirely, work for you?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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