On Wed, Jul 13, 2016 at 09:37:57PM +0200, Maxime Ripard wrote:
> 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.
Right. I can see two obvious approaches:
1. All (top-level) nodes named fragment@* are assumed to be
overlay fragments.
2. All top-evel nodes with a subnode named '__overlay__' are
assumed to be overlay fragments
(2) differs from looking for target properties because whatever
target variants we add in future, they're still likely to want an
__overlay__ node. Or at worst, we can add a dummy __overlay__ node to
them.
> 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?
I'd prefer to pick a single defining factor for the overlay fragments,
rather than a grab bag of options.
--
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:
signature.asc
Description: PGP signature
