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: 8341 bytes --]

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.

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

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

> +
> +	/* 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().

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

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

>  }
>  
>  check_tests () {
> diff --git a/tests/tests.h b/tests/tests.h
> index 578d8c1a..37ed3394 100644
> --- a/tests/tests.h
> +++ b/tests/tests.h
> @@ -109,6 +109,7 @@ const void *check_get_prop_offset(void *fdt, int poffset, const char *in_name,
>  const void *check_getprop_addrrange(void *fdt, int parent, int nodeoffset,
>  				    const char *name, int num);
>  
> +int get_poffset(void *fdt, int nodeoffset, const char *propname);
>  int nodename_eq(const char *s1, const char *s2);
>  void vg_prepare_blob(void *fdt, size_t bufsize);
>  void *load_blob(const char *filename);
> diff --git a/tests/testutils.c b/tests/testutils.c
> index 6d69e872..d35a1978 100644
> --- a/tests/testutils.c
> +++ b/tests/testutils.c
> @@ -230,6 +230,24 @@ const void *check_getprop_addrrange(void *fdt, int parent, int nodeoffset,
>  	return propval;
>  }
>  
> +int get_poffset(void *fdt, int nodeoffset, const char *propname)
> +{
> +	const char *name;
> +	int poffset;
> +
> +	fdt_for_each_property_offset(poffset, fdt, nodeoffset) {
> +		if (fdt_getprop_by_offset(fdt, poffset, &name, NULL)) {
> +			if (!strcmp(name, propname))
> +				return poffset;
> +		}
> +	}
> +
> +	if (poffset < 0)
> +		return poffset;
> +
> +	return -FDT_ERR_NOTFOUND;
> +}
> +
>  int nodename_eq(const char *s1, const char *s2)
>  {
>  	int len = strlen(s2);
> -- 
> 2.55.0
> 
> 

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