Re: [PATCH v2 17/25] binman: Add a consistent way to report errors with fit

Alper Nebi Yasak
2022-03-03T21:18:40Z

On 24/02/2022 02:00, Simon Glass wrote:
> Add a new function to handling reporting errors within a particular
> subnode of the FIT description. This can be used to make the format of
> these errors consistent.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
> (no changes since v1)
>
> tools/binman/etype/fit.py | 33 +++++++++++++++++++++++++--------
> tools/binman/ftest.py | 2 +-
> 2 files changed, 26 insertions(+), 9 deletions(-)

Reviewed-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

I don't like entry.Raise(msg) in general, if it were me I would define
Exception subclasses and e.g. raise CustomError(self, subnode) for the
various error conditions, but that's beside the point.

> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 70966028e8..50a9179f9f 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> [...]
> @@ -273,6 +274,20 @@ class Entry_fit(Entry_section):
>
> return tools.read_file(output_fname)
>
> + def _raise(self, base_node, node, msg):
> + """Raise an error with a paticular FIT subnode
> +
> + Args:
> + base_node (Node): Base Node of the FIT (with 'description' property)
> + node (Node): FIT subnode containing the error
> + msg (str): Message to report
> +
> + Raises:
> + ValueError, as requested
> + """
> + rel_path = node.path[len(base_node.path) + 1:]

Can base_node be anything except self._node here, why not use that?

> + self.Raise(f"subnode '{rel_path}': {msg}")

The names are a bit inconsistent, but I guess you intend to change
Entry.Raise() to Entry._raise() later on...

> +
> def _build_input(self):
> """Finish the FIT by adding the 'data' properties to it
>
> …