Re: [PATCH v3 11/15] libfdt: Introduce fdt_getprop_by_offset_w()

[Herve Codina]

On Thu, 17 Sep 2026 14:52:28 +1000
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:

> On Wed, Sep 16, 2026 at 12:42:05PM +0200, Herve Codina wrote:
> > Hi David,
> > 
> > On Wed, 16 Sep 2026 19:56:46 +1000
> > David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
> >   
> > > On Wed, Aug 26, 2026 at 10:31:42AM +0200, Herve Codina wrote:  
> > > > Future operations, such as handling unknown tags on dtb modifications,
> > > > need to modify properties retrieved by their offset.
> > > > 
> > > > fdt_getprop_by_offset() already exists to get a preperty by its offset
> > > > but the property returned is read-only. A writable returned property is
> > > > needed.    
> > > 
> > > Since we already have fdt_get_property_by_offset_w() this is an
> > > obvious gap and I'd be happy in principle to apply this independent of
> > > the rest of the series.  
> > 
> > Nice :)
> >   
> > >   
> > > > Fill the lack and introduce fdt_getprop_by_offset_w(), the write enabled
> > > > variant of fdt_getprop_by_offset().
> > > > 
> > > > Add also its related test.
> > > > 
> > > > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>
> > > > ---
> > > >  libfdt/libfdt.h           |  6 +++
> > > >  tests/.gitignore          |  1 +
> > > >  tests/Makefile.tests      |  2 +-
> > > >  tests/get_prop_offset_w.c | 84 +++++++++++++++++++++++++++++++++++++++
> > > >  tests/meson.build         |  1 +
> > > >  tests/run_tests.sh        |  1 +
> > > >  tests/tests.h             |  1 +
> > > >  tests/testutils.c         | 18 +++++++++
> > > >  8 files changed, 113 insertions(+), 1 deletion(-)
> > > >  create mode 100644 tests/get_prop_offset_w.c
> > > > 
> > > > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > > > index 7a1915a5..82965e32 100644
> > > > --- a/libfdt/libfdt.h
> > > > +++ b/libfdt/libfdt.h
> > > > @@ -856,6 +856,12 @@ static inline struct fdt_property *fdt_get_property_w(void *fdt, int nodeoffset,
> > > >  #ifndef SWIG /* This function is not useful in Python */
> > > >  const void *fdt_getprop_by_offset(const void *fdt, int offset,
> > > >  				  const char **namep, int *lenp);
> > > > +static inline void *fdt_getprop_by_offset_w(const void *fdt, int offset,
> > > > +					    const char **namep, int *lenp)
> > > > +{
> > > > +	return (void *)(uintptr_t)fdt_getprop_by_offset(fdt, offset, namep,
> > > > +							lenp);
> > > > +}
> > > >  #endif
> > > >  
> > > >  /**
> > > > diff --git a/tests/.gitignore b/tests/.gitignore
> > > > index 69693129..1091174a 100644
> > > > --- a/tests/.gitignore
> > > > +++ b/tests/.gitignore
> > > > @@ -30,6 +30,7 @@ tmp.*
> > > >  /get_phandle
> > > >  /getprop
> > > >  /get_prop_offset
> > > > +/get_prop_offset_w
> > > >  /incbin
> > > >  /integer-expressions
> > > >  /fs_tree1
> > > > diff --git a/tests/Makefile.tests b/tests/Makefile.tests
> > > > index 63e201ae..0d4d399f 100644
> > > > --- a/tests/Makefile.tests
> > > > +++ b/tests/Makefile.tests
> > > > @@ -16,7 +16,7 @@ LIB_TESTS_L = get_mem_rsv \
> > > >  	sw_tree1 sw_states \
> > > >  	move_and_save mangle-layout nopulate \
> > > >  	open_pack rw_tree1 rw_oom set_name setprop del_property del_node \
> > > > -	appendprop1 appendprop2 propname_escapes \
> > > > +	appendprop1 appendprop2 propname_escapes get_prop_offset_w \
> > > >  	string_escapes references path-references phandle_format \
> > > >  	boot-cpuid incbin relref_merge \
> > > >  	extra-terminating-null \
> > > > diff --git a/tests/get_prop_offset_w.c b/tests/get_prop_offset_w.c
> > > > new file mode 100644
> > > > index 00000000..dbfa5dd7
> > > > --- /dev/null
> > > > +++ b/tests/get_prop_offset_w.c
> > > > @@ -0,0 +1,84 @@
> > > > +// SPDX-License-Identifier: LGPL-2.1-or-later
> > > > +/*
> > > > + * libfdt - Flat Device Tree manipulation
> > > > + *	Testcase for fdt_getprop_by_offset_w()
> > > > + * Copyright (C) 2026 Bootlin.
> > > > + */
> > > > +
> > > > +#include <stdlib.h>
> > > > +#include <stdio.h>
> > > > +#include <string.h>
> > > > +#include <stdint.h>
> > > > +
> > > > +#include <libfdt.h>
> > > > +
> > > > +#include "tests.h"
> > > > +#include "testdata.h"
> > > > +
> > > > +#define SPACE		65536
> > > > +
> > > > +int main(int argc, char *argv[])
> > > > +{
> > > > +	const void *exp_val;
> > > > +	const char *name;
> > > > +	char *modif_val;
> > > > +	void *propval;
> > > > +	int proplen;
> > > > +	int exp_len;
> > > > +	int poffset;
> > > > +	void *fdt;
> > > > +	void *buf;
> > > > +	int err;
> > > > +
> > > > +	test_init(argc, argv);
> > > > +	fdt = load_blob_arg(argc, argv);
> > > > +
> > > > +	buf = xmalloc(SPACE);
> > > > +
> > > > +	err = fdt_open_into(fdt, buf, SPACE);    
> > > 
> > > You shouldn't need an fdt_open_into().  You're modifying the contents
> > > of a single property, but not moving tags about or (specifically)
> > > expanding the tree as a whole.  This is a "write in place" operation
> > > rather than a "read/write" operation in libfdt terms.  
> > 
> > Ok, I will update in the next iteration.
> >   
> > >   
> > > > +	if (err)
> > > > +		FAIL("fdt_open_into(): %s", fdt_strerror(err));
> > > > +
> > > > +	free(fdt);
> > > > +	fdt = buf;
> > > > +
> > > > +	/* Retrieve the offset of the 'prop-str' property */
> > > > +	poffset = get_poffset(fdt, 0, "prop-str");
> > > > +	if (poffset < 0)
> > > > +		FAIL("Property 'prop-str' not found")    
> > > 
> > > Introducing get_poffset() seems a bit awkward compared to using an
> > > fdt_for_each_property_offset() like get_prop_offset.c does.  
> > 
> > Ok, will use fdt_for_each_property_offset() directly here and remove
> > get_poffset() in the next iteration.
> >   
> > > > +
> > > > +	/* Get the property value for modification */
> > > > +	propval = fdt_getprop_by_offset_w(fdt, poffset, &name, &proplen);
> > > > +	if (!propval)
> > > > +		FAIL("fdt_getprop_by_offset_w(%d): %s", poffset,
> > > > +		     fdt_strerror(proplen));
> > > > +
> > > > +	/* Check original values */
> > > > +	if (strcmp(name, "prop-str"))
> > > > +		FAIL("Name mismatch : %s instead of prop-str", name);
> > > > +
> > > > +	exp_len = strlen(TEST_STRING_1) + 1;
> > > > +	if (proplen != exp_len)
> > > > +		FAIL("Original size mismatch on property \"%s\": %d instead of %d",
> > > > +		     name, proplen, exp_len);
> > > > +
> > > > +	exp_val = TEST_STRING_1;
> > > > +	if (memcmp(exp_val, propval, exp_len))
> > > > +		FAIL("Original data mismatch on property \"%s\"", name);
> > > > +    
> > > 
> > > You could re-use check_get_prop_offset() from testutils.c to perform
> > > these tests before invoking fdt_getprop_by_offset_w().  
> > 
> > I wanted to test that the value returned by fdt_getprop_by_offset_w() is the
> > correct one. check_get_prop_offset() calls fdt_getprop_by_offset() and not
> > fdt_getprop_by_offset_w().  
> 
> Ah, fair point.
> 
> But.. rather than checking the retreived values the same as
> get_prop_offset.c, it would be both simpler and more complete to
> explicitly check that fdt_getprop_by_offset_w() returns the same
> pointer as fdt_getprop_by_offset().

Ok, I will do that.

> 
> > I know it doesn't matter with current implementation
> > of fdt_getprop_by_offset_w() but I am not supposed to know about this
> > implementation here in the test. Also, this implementation could change in the
> > future (not planned on my side).
> >   
> > >   
> > > > +	if (exp_len < 4)
> > > > +		CONFIG("Property \"%s\" should be at least 4 bytes", name);
> > > > +
> > > > +	/* Modify the property value */
> > > > +	modif_val = xmalloc(proplen);
> > > > +	memcpy(modif_val, propval, proplen);    
> > > 
> > > Why malloc() and memcpy()?  The whole point of
> > > fdt_getprop_by_offset_w() is that you get a writable pointer into the
> > > fdt which you can modify in place.  
> > 
> > I want to keep the modified value for later comparison.
> > 
> > - modif_val : Current property val + modification.
> > - Update propval: Current property val.
> > - Check reading the property with check_get_prop_offset() that the
> >   property has been really updated.
> > 
> > If instead I use only propval, I use only one buffer and so
> > - propval = modified value
> > - check that get check_get_prop_offset() which will also return the 
> >   propval buffer matches the propval buffer.  
> 
> Ah, missed that, that makes sense.  Personally I'd probably go with a
> static buffer with the value to write in (and then compare), but
> either way is fine.

Will keep malloc()/memcpy().

The static buffer needs to know about the lengh of data, at least a
maximum allowed length.

Best regards,
Hervé