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