On 30/08/2020 00:20, Simon Glass wrote:
> On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiya...@gmail.com>
> wrote:
>> super().__init__(section, etype, node)
>> self._fit = None
>> - self._fit_content = defaultdict(list)
>> + self._fit_images = {}
>
> Can you add a Properties comment above for this?
There's already a Members comment that I forgot to adjust for the
_fit_content variable I renamed, changing it would be like this:
def __init__(self, section, etype, node):
"""
Members:
_fit: FIT file being built
- _fit_content: dict:
+ _fit_sections: dict:
key: relative path to entry Node (from the base of the FIT)
- value: List of Entry objects comprising the contents of this
+ value: Entry_section object comprising the contents of this
node
"""
super().__init__(section, etype, node)
self._fit = None
- self._fit_content = defaultdict(list)
+ self._fit_sections = {}
self._fit_props = {
Would that be enough? Putting that in the Properties sounds weird to
me since it isn't the same kind of thing as "fit,external-offset", but
section.py documents its _allow_missing in its Properties as well.
OTOH, nothing except fit.py has an __init__ docstring, so I think I can
move the entire thing into the class docstring. Either into the
Properties, or next to the Properties still under a Members heading.
What would you prefer?
>> for subnode in node.subnodes:
>> if has_images and not (subnode.name.startswith('hash') or
>>
>> subnode.name.startswith('signature')):
>> # This is a content node. We collect all of these
>> together
>> # and put them in the 'data' property. They do not
>> appear
>> # in the FIT.
>
> This comment should move along with the code you moved. Perhaps here
> you can mention that it is not a content node.
I tried to clarify and elaborate the comments a bit, because it sounded
ambiguous to me when I just moved it. How about:
has_images = depth == 2 and rel_path.startswith('/images/')
if has_images:
# This node is a FIT subimage node (e.g. "/images/kernel")
# containing content nodes. We collect the subimage nodes and
# section entries for them here to merge the content subnodes
# together and put the merged contents in the subimage node's
# 'data' property later.
entry = Entry.Create(self.section, node, etype='section')
entry.ReadNode()
self._fit_sections[rel_path] = entry
for subnode in node.subnodes:
if has_images and not (subnode.name.startswith('hash') or
subnode.name.startswith('signature')):
# This subnode is a content node not meant to appear in the
# FIT (e.g. "/images/kernel/u-boot"), so don't call
# fsw.add_node() or _AddNode() for it.
pass
else:
with fsw.add_node(subnode.name):
_AddNode(base_node, depth + 1, subnode)
>> - for path, entries in self._fit_content.items():
>> + for path, image in self._fit_images.items():
>
> I think section is a better name than image. In binman, 'image' means
> the final output file, containing a collection of binaries. The FIT
> itself therefore ends up in an image, so calling the components of the
> FIT 'images' is confusing.
I'm changing it to section and also _fit_images to _fit_sections, in
order to match that.
> Please also do check code coverage: binman test -T
Entry_section.ObtainContents() never returns False, so I'm removing the
checks for that, which contained the statements the test didn't cover.
