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

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

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

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@gibson.dropbear.id.au> 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@bootlin.com>
> > > ---
> > >  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().

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

> In check_get_prop_offset();
> --- 8< ---
> const void *check_get_prop_offset(void *fdt, int poffset, const char *exp_name,
>                                   int exp_len, const void *exp_val)
> {
> 	const void *propval;
> 	...
> 	propval = fdt_getprop_by_offset(fdt, poffset, &name, &proplen);
> 	...
>  	if (exp_len && memcmp(exp_val, propval, exp_len))
>              FAIL("Data mismatch on property \"%s\"", name);
>         ...
> }
> 
> Using a pointer returned by fdt_getprop_by_offset_w() as exp_val parameter
> leads to something like:
> 	memcmp(propval, propval, exp_len)
> 
> This is always successful even if the propval buffer doesn't contain the
> data expected by the test. If for whatever reason the propval buffer is
> wrongly modified by some internal libfdt function called by
> check_get_prop_offset(), we cannot see it.
> 
> With the additional buffer (modif_val) which is never passed to libfdt,
> and contains expected data, we can see any wrong proval modification.
> 
> > 
> > > +	modif_val[1] = (modif_val[1] != 'A') ? 'A' : 'a';
> > > +	modif_val[3] = (modif_val[3] != 'B') ? 'B' : 'b';
> > > +	memcpy(propval, modif_val, exp_len);
> > > +
> > > +	/* Check that the modified value is taken into account */
> > > +	if (!check_get_prop_offset(fdt, poffset, "prop-str", exp_len, modif_val))
> > > +		FAIL("Modified property 'prop-str' not found");
> > > +
> > > +	PASS();
> > > +}
> > > diff --git a/tests/meson.build b/tests/meson.build
> > > index 779e6bc7..dae6a941 100644
> > > --- a/tests/meson.build
> > > +++ b/tests/meson.build
> > > @@ -50,6 +50,7 @@ tests = [
> > >    'get_path',
> > >    'get_phandle',
> > >    'get_prop_offset',
> > > +  'get_prop_offset_w',
> > >    'get_next_tag_invalid_prop_len',
> > >    'getprop',
> > >    'incbin',
> > > diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> > > index 225c22f8..419a24d8 100755
> > > --- a/tests/run_tests.sh
> > > +++ b/tests/run_tests.sh
> > > @@ -419,6 +419,7 @@ tree1_tests_rw () {
> > >      run_test setprop $TREE
> > >      run_test del_property $TREE
> > >      run_test del_node $TREE
> > > +    run_test get_prop_offset_w $TREE  
> > 
> > This belongs with the other write-in-place tests in tree1_tests(), not
> > as an rw test.
> 
> Ok, I will move it.
> 
> Best regards,
> Hervé
> 

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