Re: [PATCH v2 17/25] binman: Add a consistent way to report errors with fit
Hi Alper,
On Thu, 3 Mar 2022 at 14:16, Alper Nebi Yasak wrote:
>
> 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
> > ---
> >
> > (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
>
> 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.
I have so far stuck with ValueError as I find the need to import
classes to raise an exception a pain. But I suppose that would be OK.
>
> > 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?
OK I fixed 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...
Well I was planning to change Raise() to raise(), so I'll rename this
raise_subnode()
>
> > +
> > def _build_input(self):
> > """Finish the FIT by adding the 'data' properties to it
> >
> > [...]
Regards,
Simon
Re: [PATCH v2 17/25] binman: Add a consistent way to report errors with fit
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
> ---
>
> (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
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
>
> [...]
[PATCH v2 17/25] binman: Add a consistent way to report errors with fit
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
---
(no changes since v1)
tools/binman/etype/fit.py | 33 +
tools/binman/ftest.py | 2 +-
2 files changed, 26 insertions(+), 9 deletions(-)
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
@@ -186,11 +186,11 @@ class Entry_fit(Entry_section):
def ReadNode(self):
super().ReadNode()
-def _get_operation(self, subnode):
+def _get_operation(self, base_node, node):
"""Get the operation referenced by a subnode
Args:
-subnode (Node): Subnode (of the FIT) to check
+node (Node): Subnode (of the FIT) to check
Returns:
int: Operation to perform
@@ -198,12 +198,13 @@ class Entry_fit(Entry_section):
Raises:
ValueError: Invalid operation name
"""
-oper_name = subnode.props.get('fit,operation')
+oper_name = node.props.get('fit,operation')
if not oper_name:
return OP_GEN_FDT_NODES
oper = OPERATIONS.get(oper_name.value)
-if not oper:
-self.Raise(f"Unknown operation '{oper_name.value}'")
+if oper is None:
+self._raise(base_node, node,
+f"Unknown operation '{oper_name.value}'")
return oper
def ReadEntries(self):
@@ -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:]
+self.Raise(f"subnode '{rel_path}': {msg}")
+
def _build_input(self):
"""Finish the FIT by adding the 'data' properties to it
@@ -356,7 +371,7 @@ class Entry_fit(Entry_section):
else:
self.Raise("Generator node requires 'fit,fdt-list'
property")
-def _gen_node(subnode, depth, in_images):
+def _gen_node(base_node, subnode, depth, in_images):
"""Generate nodes from a template
This creates one node for each member of self._fdts using the
@@ -366,12 +381,14 @@ class Entry_fit(Entry_section):
first.
Args:
+base_node (Node): Base Node of the FIT (with 'description'
+property)
subnode (Node): Generator node to process
depth (int): Current node depth (0 is the base 'fit' node)
in_images (bool): True if this is inside the 'images' node, so
that 'data' properties should be generated
"""
-oper = self._get_operation(subnode)
+oper = self._get_operation(base_node, subnode)
if oper == OP_GEN_FDT_NODES:
_gen_fdt_nodes(subnode, depth, in_images)
@@ -412,7 +429,7 @@ class Entry_fit(Entry_section):
elif self.GetImage().generate and subnode.name.startswith('@'):
subnode_path = f'{rel_path}/{subnode.name}'
entry = self._entries.get(subnode_path)
-_gen_node(subnode, depth, in_images)
+_gen_node(base_node, subnode, depth, in_images)
if entry:
del self._entries[subnode_path]
else:
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 106027d245..16463518aa 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5319,7 +5319,7 @@ fdt fdtmapExtract the devicetree
blob from the fdtmap
"""Check handling of an FDT map when the section cannot be found"""
with self.assertRaises(ValueError) as exc:
self._DoReadFileDtb('224_fit_bad_oper.dts')
-self.assertIn("Node '/binman/fit': Unknown operation 'unknown'",
+self.assertIn("Node '/binman/fit': subnode 'images/@fdt-SEQ': Unknown
operation 'unknown'",
str(exc.exception))
def test_uses_expand_size(self):
--
2.35.1.574.g5d30c73bfb-goog
