Re: [PATCH v2 2/2] livetree: Add only new data to fixup nodes instead of complete regeneration

David Gibson <[email protected]> Mon, 18 Aug 2025 13:06:29 +1000
Newsgroups	org.kernel.vger.devicetree-compiler
Message-ID	<aKKYtbra546AQLlz@zatzit>
On Sun, Aug 17, 2025 at 12:40:59PM +0200, Uwe Kleine-König wrote:
> On Sat, Aug 16, 2025 at 02:54:27PM +1000, David Gibson wrote:
> > On Fri, Aug 15, 2025 at 03:34:54PM +0200, Uwe Kleine-König wrote:
> > > Removing the complete __fixups__ and __local_fixups__ tree might delete
> > > data that should better be retained. See the added test for a situation
> > > that was broken before.
> > > 
> > > Note that without removing /__fixups__ and /__local_fixups__ in
> > > generate_fixups_tree() and generate_local_fixups_tree() respectively
> > > calling build_and_name_child_node() isn't safe as the nodes might
> > > already exist and then a duplicate would be added. So build_root_node()
> > > has to be used which copes correctly here.
> > > 
> > > Fixes: 915daadbb62d ("Start with empty __local_fixups__ and __fixups__ nodes")
> > > Closes: https://github.com/dgibson/dtc/issues/170
> > > Signed-off-by: Uwe Kleine-König <[email protected]>
> > > ---
> > >  livetree.c              | 75 +++++++++++++++++++++++++++++++----------
> > >  tests/retain-fixups.dts | 29 ++++++++++++++++
> > >  tests/run_tests.sh      |  5 +++
> > >  3 files changed, 92 insertions(+), 17 deletions(-)
> > >  create mode 100644 tests/retain-fixups.dts
> > > 
> > > diff --git a/livetree.c b/livetree.c
> > > index 6127d604d528..f23988483b01 100644
> > > --- a/livetree.c
> > > +++ b/livetree.c
> > > @@ -352,6 +352,60 @@ void append_to_property(struct node *node,
> > >  	p->val = data_append_data(p->val, data, len);
> > >  }
> > >  
> > > +static void append_unique_str_to_property(struct node *node,
> > > +					  char *name, const char *data, int len)
> > > +{
> > > +	struct property *p;
> > > +
> > > +	p = get_property(node, name);
> > > +	if (p) {
> > > +		if (p->val.val[p->val.len - 1] == '\0') {
> > > +			const char *s;
> > > +
> > > +			for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {
> > > +				if (strcmp(data, s) == 0)
> > > +					/* data already contained in node.name */
> > > +					return;
> > > +			}
> > > +		} else {
> > > +			fprintf(stderr, "Warning: appending string to non-string property %s/%s\n",
> > > +				node->fullpath, name);
> > 
> > I don't think carrying on with the operation when the original
> > property is malformed is a good idea.  This message will also be
> > pretty confusing in the one place it will occur.  I think it would be
> > preferable to return an error code here.  The caller, which knows what
> > the purpose is can then report that the relevant property is malformed
> > - if that's the case, I think we can just give up on updating
> > __fixups__ / __local_fixups__.
> 
> I picked that option to somewhat keep the behaviour of dtc as it was
> before (which appended irrespective of the pre-existing content).

The rationale make sense, but I think just giving an error is a more
useful behaviour over all.

> But I don't feel strong and can rework it accordingly to your comments.
> 
> Best regards
> Uwe



-- 
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
signature.asc (application/pgp-signature, 833 B)