Re: [PATCH v3 03/18] dtc: Add a struct for the top-level fragments of a dts file

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

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

On Fri, Nov 06, 2026 at 12:51:30PM -0700, Simon Glass wrote:
> A dts file is made up of top-level fragments: the root node, later
> definitions of a node by label or path, and /delete-node/ or
> /omit-if-no-ref/ with a reference. We would like to deal with each
> fragment on its own before it is merged, for example to apply deletes
> consistently or to run some checks on each one, but that requires
> keeping the fragments apart. At present the parser merges each one into
> the tree as soon as it is parsed.
> 
> Add a struct fragment to hold one of these, with a function to build
> each kind, and functions to append one to a list and to free it.
> 
> These are not used yet.
> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> David, you asked why the root needs a special type, rather than setting
> ref to "/". This drops the type, but uses a NULL ref rather than "/",
> since the two differ in a plugin: / { ... } adds to the plugin's own
> root, whereas &{/} { ... } becomes a fragment@N node for the root of the
> base tree. Using "/" for both would change the output.
> merge-root-plugin.dts checks this.
> 
> (no changes since v2)
> 
> Changes in v2:
> - Replace FRAGMENT_ROOT and FRAGMENT_REF with FRAGMENT_NODE
> - Use a NULL ref for the root node
> - Drop path_ref in favour of checking the ref for a leading '/'
> - Add a constructor for each kind of fragment, taking all its fields
> - Add the list to struct dt_info later, when it is first used
> - Change the subject from 'Add a list' to 'Add a struct'
> 
>  dtc.h      | 50 ++++++++++++++++++++++++++++++++
>  livetree.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 135 insertions(+)
> 
> diff --git a/dtc.h b/dtc.h
> index 473552e..f15a162 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -322,6 +322,56 @@ struct reserve_info *chain_reserve_entry(struct reserve_info *first,
>  struct reserve_info *add_reserve_entry(struct reserve_info *list,
>  				       struct reserve_info *new);
>  
> +/*
> + * A fragment is one top-level part of a dts file, after the header and any
> + * memory reservations: the root node, a later definition of a node which
> + * refers to it by label or path, or a /delete-node/ or /omit-if-no-ref/ with
> + * a reference. Fragments are kept apart until they are merged into a single
> + * tree.
> + *
> + * This is not the same as an overlay fragment (a fragment@N node), although
> + * in a plugin a fragment may become one when it is merged.
> + */
> +
> +enum fragment_type {
> +	FRAGMENT_NODE,		/* / { ... }; or [label:] &ref { ... }; */
> +	FRAGMENT_DELETE_NODE,	/* /delete-node/ &ref; */
> +	FRAGMENT_OMIT_IF_NO_REF, /* /omit-if-no-ref/ &ref; */
> +};
> +
> +/*
> + * Which fields are used depends on the type:
> + *
> + *   FRAGMENT_NODE            node, plus ref and ref_srcpos unless it is the
> + *                            root node, plus label and label_srcpos if there
> + *                            is a label
> + *   FRAGMENT_DELETE_NODE     ref and ref_srcpos
> + *   FRAGMENT_OMIT_IF_NO_REF  ref and ref_srcpos
> + *
> + * Unused fields are NULL. For / { ... }; the ref is NULL rather than "/", since
> + * in a plugin &{/} { ... }; refers to the root of the base tree instead.

With this version, I think these fields are clearer, and there's less
need for describing this.  All fields are always "valid" in the sense
that they're initialized to something sensible, though they might be
NULL in some cases (and must be for certain types).

> + */
> +struct fragment {
> +	enum fragment_type type;
> +	char *ref;			/* label or path referred to */
> +	struct srcpos *ref_srcpos;	/* position of ref */
> +	char *label;			/* label to add to the target */
> +	struct srcpos *label_srcpos;	/* position of label */

What's label_srcpos used for, distinct from ref_srcpos?  The'll almost
always be in adjacent places on the same line.

> +	struct node *node;		/* the node definition */
> +
> +	struct fragment *next;		/* NULL at the end of the list */
> +};
> +
> +struct fragment *build_fragment_root(struct node *node);
> +struct fragment *build_fragment_ref(char *label, struct srcpos *label_srcpos,
> +				    char *ref, struct srcpos *ref_srcpos,
> +				    struct node *node);
> +struct fragment *build_fragment_delete_node(char *ref,
> +					    struct srcpos *ref_srcpos);
> +struct fragment *build_fragment_omit_if_no_ref(char *ref,
> +					       struct srcpos *ref_srcpos);
> +struct fragment *append_fragment(struct fragment *list, struct fragment *new);
> +void free_fragment(struct fragment *f);
>  
>  struct dt_info {
>  	unsigned int dtsflags;
> diff --git a/livetree.c b/livetree.c
> index abe73de..85725d3 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -448,6 +448,91 @@ struct reserve_info *add_reserve_entry(struct reserve_info *list,
>  	return list;
>  }
>  
> +/* Create a fragment of the given type, with its other fields NULL */
> +static struct fragment *new_fragment(enum fragment_type type, char *ref,
> +				     struct srcpos *ref_srcpos)
> +{
> +	struct fragment *new = xmalloc(sizeof(*new));
> +
> +	memset(new, '\0', sizeof(*new));

I'd use 0 rather than '\0' here - I'd only use the latter when the
thing we're zeroing is a string buffer.

> +
> +	new->type = type;
> +	new->ref = ref;
> +	new->ref_srcpos = srcpos_copy(ref_srcpos);
> +
> +	return new;
> +}
> +
> +/* Create a fragment for / { ... }; */
> +struct fragment *build_fragment_root(struct node *node)
> +{
> +	struct fragment *new = new_fragment(FRAGMENT_NODE, NULL, NULL);

I think this should also take a srcpos: after all the fragment itself
(and the literal "/") does have a position in the source, even if it
isn't a ref token.  I'm ok if the caller passes NULL for now, if it's
awkward to extract the srcpos at present.

In fact, I think the root fragment could be treated as a special case
of a ref fragment (with ref == NULL, for now), so having this call
build_fragment_ref would make sense.

> +
> +	new->node = node;
> +
> +	return new;
> +}
> +
> +/*
> + * Create a fragment for [label:] &ref { ... }; where label and label_srcpos are
> + * NULL if there is no label
> + */
> +struct fragment *build_fragment_ref(char *label, struct srcpos *label_srcpos,
> +				    char *ref, struct srcpos *ref_srcpos,
> +				    struct node *node)
> +{
> +	struct fragment *new = new_fragment(FRAGMENT_NODE, ref, ref_srcpos);
> +
> +	new->label = label;
> +	new->label_srcpos = srcpos_copy(label_srcpos);
> +	new->node = node;
> +
> +	return new;
> +}
> +
> +/* Create a fragment for /delete-node/ &ref; */
> +struct fragment *build_fragment_delete_node(char *ref,
> +					    struct srcpos *ref_srcpos)
> +{
> +	return new_fragment(FRAGMENT_DELETE_NODE, ref, ref_srcpos);
> +}
> +
> +/* Create a fragment for /omit-if-no-ref/ &ref; */
> +struct fragment *build_fragment_omit_if_no_ref(char *ref,
> +					       struct srcpos *ref_srcpos)
> +{
> +	return new_fragment(FRAGMENT_OMIT_IF_NO_REF, ref, ref_srcpos);
> +}
> +
> +/* Add a fragment to the end of a list, returning the new head */
> +struct fragment *append_fragment(struct fragment *list, struct fragment *new)

Since this is used from the parser, I think we can make this a bit
more similar to existing code as a quasi-functional chain_fragment(),
similar to the existing chain_node().  Technically that also removes
an O(n^2) behaviour, although not with an n that's likely to matter.

> +{
> +	struct fragment *last;
> +
> +	new->next = NULL;
> +
> +	if (!list)
> +		return new;
> +
> +	for (last = list; last->next; last = last->next)
> +		;
> +
> +	last->next = new;
> +
> +	return list;
> +}
> +
> +/*
> + * Free a fragment, but not its node, reference or label, since the tree may
> + * still use these
> + */
> +void free_fragment(struct fragment *f)
> +{
> +	srcpos_free(f->ref_srcpos);
> +	srcpos_free(f->label_srcpos);
> +	free(f);
> +}
> +
>  static void fill_fullpaths(struct node *tree, const char *prefix)
>  {
>  	struct node *child;
> -- 
> 2.43.0
> 
> 

-- 
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 --]