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

Uwe Kleine-König <[email protected]> Thu, 14 Aug 2025 11:05:00 +0200
Newsgroups	org.kernel.vger.devicetree-compiler
Message-ID	<eoqc474y7alu7bbofhkilh2urifqjzj2bimtmetctqcho3usxe@ncwxjr6v2mps>
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.

> > +			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.
 
> > @@ -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.

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