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

[Simon Glass]

Hi Alper,

On Sun, 30 Aug 2020 at 12:17, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> 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.

That's fine.

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

Up to you, so long as it is documented.

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

Looks good!

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

OK

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

If you put something in the FIT that depends on something else, it
will return False on the first pass. So you can't remove that code.

Instead, use the _testing etype with a 'return-contents-later'
property. to ensure the path is testing. See 162_fit_external.dts for
how this works.

Regards,
Simon