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

David Gibson <[email protected]> Fri, 15 Aug 2025 14:08:56 +1000
Newsgroups	org.kernel.vger.devicetree-compiler
Message-ID	<aJ6y2EMdqwsPByWY@zatzit>
On Thu, Aug 14, 2025 at 11:05:00AM +0200, Uwe Kleine-König wrote:
> Hello David,
> 
> On Thu, Aug 14, 2025 at 05:36:08PM +1000, David Gibson wrote:
> > On Fri, Aug 01, 2025 at 06:00:31PM +0200, Uwe Kleine-König wrote:
> > >  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 d51d05830b18..24f7c561d77e 100644
> > > --- a/livetree.c
> > > +++ b/livetree.c
> > > @@ -356,6 +356,60 @@ void append_to_property(struct node *node,
> > >  	}
> > >  }
> > >  
> > > +static void append_unique_str_to_property(struct node *node,
> > > +					  char *name, const char *data, int len)
> > > +{
> > > +	struct data d;
> > > +	struct property *p;
> > > +
> > > +	p = get_property(node, name);
> > > +	if (p) {
> > > +		const char *s;
> > > +
> > > +		for (s = p->val.val; s < p->val.val + p->val.len; s = strchr(s, '\0') + 1) {
> > 
> > This isn't quite safe.  You check s is within bounds on each
> > iteration, but if the property is malformed and doesn't end with a \0,
> > the strchr() itself could read beyond the property's bounds.
> > 
> > strnchr() could work, but is awkward: it would return NULL if there
> > are no further \0 within the property, and on most systems NULL <
> > p->val.val + p->val.len would return true, so you'd have to check for
> > that case separately.  You could use strnlen() instead, but that's
> > also a bit awkward - it doesn't include the \0, so you'd need to add
> > +1 - but in the malformed case that would put you one beyond the end
> > of the buffer.  That would probably work in practice, but creating
> > pointers beyond the buffer they're within is technically UB.
> 
> fair, my approach would be to check p->val.val[p->val.len - 1] == '\0'
> once before the loop.

Ah, yes, that's an easier way to do it.

> > > +			if (strcmp(data, s) == 0)
> > 
> > This also relies on the \0 being there.  Any fix for the above, will
> > probably result in being able to get the length of each string segment
> > fairly naturally, so it would make sense to use memcmp() instead.
> > 
> > > +				/* data already contained in node.name */
> > > +				return;
> > > +		}
> > > +
> > > +		d = data_add_marker(p->val, TYPE_STRING, name);
> > > +		d = data_append_data(d, data, len);
> > > +		p->val = d;
> > > +	} else {
> > > +		d = data_add_marker(empty_data, TYPE_STRING, name);
> > > +		d = data_append_data(d, data, len);
> > > +		p = build_property(name, d, NULL);
> > > +		add_property(node, p);
> > 
> > You can add the property first, then share the data_append_data()
> > logic with the previous case.
> 
> This is mostly taken from append_to_property(), I will check if your
> suggested improvement will apply to that one, too.

Ok, thanks.

> > > @@ -1056,29 +1110,16 @@ void generate_label_tree(struct dt_info *dti, const char *name, bool allocph)
> > >  
> > >  void generate_fixups_tree(struct dt_info *dti, const char *name)
> > >  {
> > > -	struct node *n = get_subnode(dti->dt, name);
> > > -
> > > -	/* Start with an empty __fixups__ node to not get duplicates */
> > > -	if (n)
> > > -		n->deleted = true;
> > > -
> > >  	if (!any_fixup_tree(dti, dti->dt))
> > >  		return;
> > > -	generate_fixups_tree_internal(dti,
> > > -				      build_and_name_child_node(dti->dt, name),
> > > +	generate_fixups_tree_internal(dti, build_root_node(dti->dt, name),
> > >  				      dti->dt);
> > 
> > It's not obvious to me why this change follows from the rest.
> 
> The relevant difference here is that now it's unknown if the __fixups__
> node already exists. build_and_name_child_node() only works if it
> doesn't exist which isn't ensured now any more with the lines deleted
> above.
> 
> This is a revert of 915daadbb62d.

Ah, yes I see.  Thanks.

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