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

On Thu, Jul 14, 2016 at 9:30 AM, David Gibson
[off-list ref] wrote:

On Wed, Jul 13, 2016 at 09:37:57PM +0200, Maxime Ripard wrote:

quoted

On Thu, Jul 14, 2016 at 01:07:45AM +1000, David Gibson wrote:

quoted

On Wed, Jul 13, 2016 at 10:38:03AM +0200, Maxime Ripard wrote:

quoted

Hi David,

On Wed, Jul 13, 2016 at 12:34:04AM +1000, David Gibson wrote:

quoted

On Mon, Jul 11, 2016 at 09:20:44PM +0100, Phil Elwell wrote:

quoted

On 11/07/2016 20:56, Maxime Ripard wrote:

[snip]

quoted

quoted

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

quoted

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.

quoted

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.

I think it's potentially dangerous using the presence of a particular
property to identify an overlay - what if the property name ("target",
say) was also the name of a label or property within one of the
fragments? It could then appear in the __symbols__ or __fixups__ node,
causing it to be treated as a potential overlay, but application will
then be halted with an error because it has no __overlay__ subnode.

If we make the presence of the __overlay__ subnode the decision point
(which is nicer anyway because it is a single test rather than two -
"target" and "target-path") then we avoid this hazard - __symbols__
and __fixups__ have no subnodes, and __local_fixups__ only has
top-level nodes as its subnodes, meaning __overlay__ will always be
one level further down.

Phil - Raspberry Pi