Re: [PATCH v2 3/4] fdtput: Suggest -p when a node is missing

[Herve Codina <herve.codina@bootlin.com>]

Hi Simon,

On Wed, 28 Oct 2026 01:34:19 -0600
Simon Glass <sjg@chromium.org> wrote:

> When the node being written to does not exist, fdtput just prints
> FDT_ERR_NOTFOUND - nothing points to -p, which creates missing nodes, so
> it is easy to conclude that fdtput cannot add nodes at all.
> 
> When creating a node with -c, the error also names the full path, even
> though it is the parent which is missing.
> 
> Add a hint to use -p when a node is missing, both when writing a
> property and when creating a node. Report the parent path when it is
> the parent which is missing.
> 
> Add tests that writing a property or creating a node fails when a parent
> node is missing and -p is not given, and that the error suggests -p.
> Add a run_wrap_error_msg_test() helper for this, which checks the
> output as well as the return code.

Nice idea!

> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Check that the error suggests -p, using a new helper
> - Drop David's Reviewed-by, since the tests have changed
> 
>  fdtput.c           | 20 ++++++++++++++++++--
>  tests/run_tests.sh | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/fdtput.c b/fdtput.c
> index 3c6cc98..b8d5081 100644
> --- a/fdtput.c
> +++ b/fdtput.c

...

I am ok with the implementation in fdtput.c.


> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index b3d3f3a..cf978ee 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -181,6 +181,37 @@ run_wrap_error_test () {
>      base_run_test wrap_error "$@"
>  }
>  
> +# Run a command, checking that it fails and that its output has $1 in it
> +wrap_error_msg () {
> +    (
> +	msg="$1"
> +	shift
> +	log=tmp.log.$$
> +	trap "rm -f $log" 0
> +	verbose_run_log "$log" "$@"
> +	ret="$?"
> +	FAIL_IF_SIGNAL $ret
> +	if [ "$ret" -eq 0 ]; then
> +	    FAIL "Expected non-zero return code"
> +	fi
> +	if ! grep -qF -- "$msg" "$log"; then
> +	    FAIL "Expected message '$msg'"
> +	fi
> +	PASS
> +    )
> +}
> +
> +# Check that a command fails and prints a message
> +# $1: message to look for in the output
> +# $2...: command to run
> +run_wrap_error_msg_test () {
> +    msg="$1"
> +    shift
> +    shorten_echo "$@"
> +    printf " {!= 0, \"%s\"}:	" "$msg"
> +    base_run_test wrap_error_msg "$msg" "$@"
> +}
> +

Right, you introduce this wrapper and I think they can be useful for other
cases.

You also sent the series "dtc: Fix deletes within a section and before a
redefinition" [1].

The patch "tests: Allow dtc-fatal.sh to check the error message" [2]
available in the series plays also with error messages, adding a "-m" option
to tests/dtc-fatal.sh.

At some point, could all tests use the same uniform method (probably the
wrappers added here)?

Could dtc-fatal.sh script be replaced by an existing wrapper?

Again, I think wrappers you added here are nice. I would like to have your
opinion related to dtc-fatal.sh and a way to avoid the '-m' option in order
to have uniform features.

[1]: https://lore.kernel.org/devicetree-compiler/20261028093620.3705074-1-sjg@chromium.org/
[2]: https://lore.kernel.org/devicetree-compiler/20261028093620.3705074-5-sjg@chromium.org/

>  # $1: dtb file
>  # $2: align base
>  check_align () {
> @@ -962,6 +993,12 @@ fdtput_tests () {
>      run_wrap_error_test $DTPUT $dtb -c /chosen
>      run_wrap_error_test $DTPUT $dtb -c /chosen/son
>  
> +    # Writing below a missing node fails without -p, suggesting -p
> +    run_wrap_error_msg_test "Use -p to create missing nodes" \
> +	$DTPUT $dtb -c /blackadder/the-second
> +    run_wrap_error_msg_test "Use -p to create missing nodes" \
> +	$DTPUT $dtb /blackadder/the-second cost -ts high
> +
>      # Automatic node creation
>      run_wrap_test $DTPUT $dtb -cp /blackadder/the-second/turnip \
>  	/blackadder/the-second/potato

Best regards,
Hervé