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

Uwe Kleine-König <[email protected]> Sun, 17 Aug 2025 12:40:59 +0200

Newsgroups	org.kernel.vger.devicetree-compiler
Message-ID	<2jywkljpilxb654diunhguk7apu5st2cotkgjjvyn25cpq6slx@e5ufgjcipzez>

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

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

Best regards
Uwe

signature.asc (application/pgp-signature, 488 B)