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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4453 bytes --]

On Thu, Aug 14, 2025 at 11:28:05AM +0200, Uwe Kleine-König wrote:
> 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:

Looks good.

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

You could even do this in place in p->val, avoiding the temporary.

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



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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]