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

[Herve Codina <herve.codina@bootlin.com>]

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().

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