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 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 > > > --- > > > 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 > > > +#include > > > +#include > > > +#include > > > + > > > +#include > > > + > > > +#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