[PATCH 3/3] binman: Build FIT image subentries with the section etype

[Alper Nebi Yasak]

On 30/08/2020 00:20, Simon Glass wrote:
> On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiyasak at 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.