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

On Thu, Aug 14, 2025 at 11:05:02AM +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.
> 
> > > +			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.

I think that would be:

diff --git a/livetree.c b/livetree.c
index d51d05830b18..2ccdc9d9c2a7 100644
--- a/livetree.c
+++ b/livetree.c
@@ -344,16 +344,14 @@ void append_to_property(struct node *node,
 	struct property *p;
 
 	p = get_property(node, name);
-	if (p) {
-		d = data_add_marker(p->val, type, name);
-		d = data_append_data(d, data, len);
-		p->val = d;
-	} else {
-		d = data_add_marker(empty_data, type, name);
-		d = data_append_data(d, data, len);
-		p = build_property(name, d, NULL);
+	if (!p) {
+		p = build_property(name, empty_data, NULL);
 		add_property(node, p);
 	}
+
+	d = data_add_marker(p->val, type, name);
+	d = data_append_data(d, data, len);
+	p->val = d;
 }
 
 struct reserve_info *build_reserve_entry(uint64_t address, uint64_t size)

The test suite is happy with it. I will include that with the next
revision of my fix patch.

Best regards
Uwe

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmidrCIACgkQj4D7WH0S
/k7qZwf+NrKv/v0OSHXcIrdsqjDTXDZDrBY2I+w8e0G80zL/IIE7JW8qjDXycreJ
SG5F2zPPS0scJbOkJ8JcSUjawJUZbekQj4qYsndPYbVFoWRCQJJGXeFvgcaW09Rf
UdX46XZnMFyC8O/iFQ1Up1WZ+3D7suCbEkwXgYfUGDKr85On1qO/sQ30U6RNNI5P
xIcbsJl/OKurUp/DkSQ6OwKNBciO5CW35Q8H+b7kti749lP08YxyzltXq/d1v6f8
wPhA3IrYDazbSUPX2qyovae0lebHZmWuM8oRajKC4gL6SBmaB0lNLy1BqVlqAzFM
SuFC639tTonQb437MRl5KY5DIpsGDQ==
=c9jj
-----END PGP SIGNATURE-----