Re: [PATCH 11/13] dtc: Add helpers for the errors when merging a fragment

[David Gibson <david@gibson.dropbear.id.au>]

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

On Tue, Nov 03, 2026 at 06:23:44PM +0100, Herve Codina wrote:
> On Sun,  1 Nov 2026 09:37:05 -0700
> Simon Glass <sjg@chromium.org> wrote:
> 
> > Four of the merge functions look up the node which a fragment refers to
> > and report 'Label or path not found' if it is missing, and three check
> > for a label-relative reference in a plugin. Each of these has its own
> > copy of the error, so a change to a message or its position must be made
> > in several places.
> > 
> > Add lookup_ref() to look up the node, reporting an error if it is not
> > found, and check_relative_ref() to report a label-relative reference.
> > Use these in the merge functions.
> > 
> > The output does not change.
> > 
> > Assisted-by: Claude:claude-opus-5-5
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> > 
> >  livetree.c | 90 +++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 49 insertions(+), 41 deletions(-)
> > 
> > diff --git a/livetree.c b/livetree.c
> > index 6a290fb..1e012d4 100644
> > --- a/livetree.c
> > +++ b/livetree.c
> > @@ -548,6 +548,40 @@ static bool is_ref_relative(const char *ref)
> >  	return ref[0] != '/' && strchr(&ref[1], '/');
> >  }
> >  
> > +/*
> > + * Report an error at pos if a fragment uses a label-relative reference, which
> > + * a plugin does not support
> > + *
> > + * Returns true if there is an error
> > + */
> > +static bool check_relative_ref(struct fragment *f, struct srcpos *pos)
> > +{
> > +	if (!is_ref_relative(f->ref))
> > +		return false;
> > +	srcpos_error(pos, "Error",
> > +		     "Label-relative reference %s not supported in plugin",
> > +		     f->ref);
> 
> You suppose that srcpos is valid and so this function and remaining ones are
> be called only from dt_from_source() with the parsing done by dtc-parser.y.
> 
> Not sure that this is a good assumption for the future.
> 
> IHMO, it will be better to decouple those functions from the type of parsing
> or just add a simple 'if (pos)' before calling srcpos_error().

It's a good point that eventually we'll want to get fragments from
dtbos as well as from source, in which case we won't have srcpos
information.  But when we are coming from source, the position
information is valuable.

This case of maybe-present srcpos information is common enough, that
the way to address it might be to alter srcpos_string() to accept
NULL, returning "<unknown>" or some other suitable placeholder when
srcpos information isn't available.

> 
> Best regards,
> Hervé
> 
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * Look up the node which a fragment refers to, reporting an error if it is
> > + * not found
> > + *
> > + * Returns the node, or NULL if it is not found
> > + */
> > +static struct node *lookup_ref(struct node *dt, struct fragment *f)
> > +{
> > +	struct node *target = get_node_by_ref(dt, f->ref);
> > +
> > +	if (!target)
> > +		srcpos_error(f->ref_srcpos, "Error",
> > +			     "Label or path %s not found", f->ref);
> > +
> > +	return target;
> > +}
> > +
> >  /*
> >   * Make the tree at *dtp from a first fragment which refers to a label or path,
> >   * rather than being the root node. This is only valid in a plugin, where the
> > @@ -557,17 +591,14 @@ static bool is_ref_relative(const char *ref)
> >   */
> >  static bool merge_first_ref(struct node **dtp, struct fragment *f, bool plugin)
> >  {
> > -	bool error = false;
> > +	bool error;
> >  
> > -	if (!plugin) {
> > +	if (plugin) {
> > +		error = check_relative_ref(f, f->node->srcpos);
> > +	} else {
> >  		srcpos_error(f->node->srcpos, "Error",
> >  			     "Label or path %s not found", f->ref);
> >  		error = true;
> > -	} else if (is_ref_relative(f->ref)) {
> > -		srcpos_error(f->node->srcpos, "Error",
> > -			     "Label-relative reference %s not supported in plugin",
> > -			     f->ref);
> > -		error = true;
> >  	}
> >  	/* Make a new, empty tree with a fragment@N node for the reference */
> >  	*dtp = add_orphan_node(name_node(build_node(NULL, NULL, NULL), ""),
> > @@ -584,20 +615,12 @@ static bool merge_first_ref(struct node **dtp, struct fragment *f, bool plugin)
> >   */
> >  static bool merge_labelled_ref(struct node *dt, struct fragment *f, bool plugin)
> >  {
> > -	struct node *target = get_node_by_ref(dt, f->ref);
> > -	bool error = false;
> > +	struct node *target;
> > +	bool error = plugin && check_relative_ref(f, f->label_srcpos);
> >  
> > -	if (plugin && is_ref_relative(f->ref)) {
> > -		srcpos_error(f->label_srcpos, "Error",
> > -			     "Label-relative reference %s not supported in plugin",
> > -			     f->ref);
> > -		error = true;
> > -	}
> > -	if (!target) {
> > -		srcpos_error(f->ref_srcpos, "Error",
> > -			     "Label or path %s not found", f->ref);
> > +	target = lookup_ref(dt, f);
> > +	if (!target)
> >  		return true;
> > -	}
> >  	add_label(&target->labels, f->label);
> >  	merge_nodes(target, f->node);
> >  
> > @@ -615,26 +638,17 @@ static bool merge_path_ref(struct node *dt, struct fragment *f, bool plugin)
> >  	struct node *target;
> >  
> >  	if (plugin) {
> > -		bool error = false;
> > +		bool error = check_relative_ref(f, f->ref_srcpos);
> >  
> > -		if (is_ref_relative(f->ref)) {
> > -			srcpos_error(f->ref_srcpos, "Error",
> > -				     "Label-relative reference %s not supported in plugin",
> > -				     f->ref);
> > -			error = true;
> > -		}
> >  		/* Add a fragment@N node which applies the node at the path */
> >  		add_orphan_node(dt, f->node, f->ref);
> >  
> >  		return error;
> >  	}
> >  
> > -	target = get_node_by_ref(dt, f->ref);
> > -	if (!target) {
> > -		srcpos_error(f->ref_srcpos, "Error",
> > -			     "Label or path %s not found", f->ref);
> > +	target = lookup_ref(dt, f);
> > +	if (!target)
> >  		return true;
> > -	}
> >  	merge_nodes(target, f->node);
> >  
> >  	return false;
> > @@ -672,13 +686,10 @@ static bool merge_label_ref(struct node *dt, struct fragment *f, bool plugin)
> >   */
> >  static bool merge_delete_node(struct node *dt, struct fragment *f)
> >  {
> > -	struct node *target = get_node_by_ref(dt, f->ref);
> > +	struct node *target = lookup_ref(dt, f);
> >  
> > -	if (!target) {
> > -		srcpos_error(f->ref_srcpos, "Error",
> > -			     "Label or path %s not found", f->ref);
> > +	if (!target)
> >  		return true;
> > -	}
> >  	delete_node(target);
> >  
> >  	return false;
> > @@ -692,13 +703,10 @@ static bool merge_delete_node(struct node *dt, struct fragment *f)
> >   */
> >  static bool merge_omit_if_no_ref(struct node *dt, struct fragment *f)
> >  {
> > -	struct node *target = get_node_by_ref(dt, f->ref);
> > +	struct node *target = lookup_ref(dt, f);
> >  
> > -	if (!target) {
> > -		srcpos_error(f->ref_srcpos, "Error",
> > -			     "Label or path %s not found", f->ref);
> > +	if (!target)
> >  		return true;
> > -	}
> >  	omit_node_if_unused(target);
> >  
> >  	return false;
> 
> 
> 
> -- 
> Hervé Codina, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

-- 
David Gibson (he or they)	| 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: 833 bytes --]