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

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

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

On Wed, Oct 28, 2026 at 10:08:54PM +0100, Herve Codina wrote:
> 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.

Yes, it would be good to consolidate.  For now, this series is ready
and the other still needs review, so I think we make it adjust to the
changes from this one, rather than the other way around.

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