[PATCH 0/3] binman: Make FIT image subentries respect offset, alignment and padding

[PATCH 0/3] binman: Make FIT image subentries respect offset, alignment and padding

[Alper Nebi Yasak]

I've been automating the process in doc/README.chromium-chainload and
while experimenting with whether a "kernel" image with u-boot-spl and
u-boot would work, noticed I couldn't align/offset/pad the two parts.

E.g. in something like the following, binman doesn't add the necessary
padding to place the "u-boot" to the correct offset within the
"kernel-1" data:

	fit {
		description = "example";

		images {
			kernel-1 {
				description = "U-Boot with SPL";
				type = "kernel";
				arch = "arm64";
				os = "linux";
				compression = "none";

				u-boot-spl {
				};
				u-boot {
					offset = <CONFIG_SPL_PAD_TO>;
				};
			};
		};
	};

Not sure if that'll ever be really necessary besides my experiment, but
it doesn't seem like skipping the padding was a deliberate choice, so
here are some fixes I wrote for that.


Alper Nebi Yasak (3):
  binman: Ignore hash*, signature* nodes in sections
  binman: Respect pad-before property of section subentries
  binman: Build FIT image subentries with the section etype

 tools/binman/etype/fit.py                     | 22 +++----
 tools/binman/etype/section.py                 |  4 +-
 tools/binman/ftest.py                         | 32 +++++++++++
 tools/binman/test/165_pad_in_sections.dts     | 26 +++++++++
 .../test/166_fit_image_subentry_alignment.dts | 57 +++++++++++++++++++
 5 files changed, 129 insertions(+), 12 deletions(-)
 create mode 100644 tools/binman/test/165_pad_in_sections.dts
 create mode 100644 tools/binman/test/166_fit_image_subentry_alignment.dts

-- 
2.28.0

[PATCH 1/3] binman: Ignore hash*, signature* nodes in sections

[Alper Nebi Yasak]

Switch to str.startswith for matching like the FIT etype does since the
current version doesn't ignore 'hash-1', 'hash-2', etc.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/etype/section.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index 73c5553c81..c5166a5b57 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -83,7 +83,7 @@ class Entry_section(Entry):
 
     def _ReadEntries(self):
         for node in self._node.subnodes:
-            if node.name == 'hash':
+            if node.name.startswith('hash') or node.name.startswith('signature'):
                 continue
             entry = Entry.Create(self, node)
             entry.ReadNode()
-- 
2.28.0

[PATCH 2/3] binman: Respect pad-before property of section subentries

[Alper Nebi Yasak]

Other relevant properties (pad-after, offset, size, align, align-size,
align-end) already work since Pack() sets correct ranges for subentries'
data (.offset, .size variables), but some padding here is necessary to
align the data within this range to match the pad-before property.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/etype/section.py             |  2 +-
 tools/binman/ftest.py                     |  8 +++++++
 tools/binman/test/165_pad_in_sections.dts | 26 +++++++++++++++++++++++
 3 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 tools/binman/test/165_pad_in_sections.dts

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
index c5166a5b57..72600b1ef3 100644
--- a/tools/binman/etype/section.py
+++ b/tools/binman/etype/section.py
@@ -152,7 +152,7 @@ class Entry_section(Entry):
         for entry in self._entries.values():
             data = entry.GetData()
             base = self.pad_before + (entry.offset or 0) - self._skip_at_start
-            pad = base - len(section_data)
+            pad = base - len(section_data) + (entry.pad_before or 0)
             if pad > 0:
                 section_data += tools.GetBytes(self._pad_byte, pad)
             section_data += data
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 5f650b5f94..8edf85c89f 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -1269,6 +1269,14 @@ class TestFunctional(unittest.TestCase):
                     U_BOOT_DATA + tools.GetBytes(ord('&'), 4))
         self.assertEqual(expected, data)
 
+    def testPadInSections(self):
+        """Test pad-before, pad-after for entries in sections"""
+        data = self._DoReadFile('165_pad_in_sections.dts')
+        expected = (U_BOOT_DATA + tools.GetBytes(ord('!'), 12) +
+                    U_BOOT_DATA + tools.GetBytes(ord('!'), 6) +
+                    U_BOOT_DATA)
+        self.assertEqual(expected, data)
+
     def testMap(self):
         """Tests outputting a map of the images"""
         _, _, map_data, _ = self._DoReadFileDtb('055_sections.dts', map=True)
diff --git a/tools/binman/test/165_pad_in_sections.dts b/tools/binman/test/165_pad_in_sections.dts
new file mode 100644
index 0000000000..f2b327ff9f
--- /dev/null
+++ b/tools/binman/test/165_pad_in_sections.dts
@@ -0,0 +1,26 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		pad-byte = <0x26>;
+		section {
+			pad-byte = <0x21>;
+
+			before {
+				type = "u-boot";
+			};
+			u-boot {
+				pad-before = <12>;
+				pad-after = <6>;
+			};
+			after {
+				type = "u-boot";
+			};
+		};
+	};
+};
-- 
2.28.0

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

[Alper Nebi Yasak]

When reading subentries of each image, the FIT entry type directly
concatenates their contents without padding them according to their
offset, size, align, align-size, align-end, pad-before, pad-after
properties.

This patch makes sure these properties are respected by offloading this
image-data building to the section etype, where each subnode of the
"images" node is processed as a section. Alignments and offsets are
respective to the beginning of each image. For example, the following
fragment can end up having "u-boot-spl" start at 0x88 within the final
FIT binary, while "u-boot" would then end up starting at e.g. 0x20088.

	fit {
		description = "example";

		images {
			kernel-1 {
				description = "U-Boot with SPL";
				type = "kernel";
				arch = "arm64";
				os = "linux";
				compression = "none";

				u-boot-spl {
				};
				u-boot {
					align = <0x10000>;
				};
			};
		};
	}

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

 tools/binman/etype/fit.py                     | 22 +++----
 tools/binman/ftest.py                         | 24 ++++++++
 .../test/166_fit_image_subentry_alignment.dts | 57 +++++++++++++++++++
 3 files changed, 93 insertions(+), 10 deletions(-)
 create mode 100644 tools/binman/test/166_fit_image_subentry_alignment.dts

diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
index 75712f4409..f136a2c254 100644
--- a/tools/binman/etype/fit.py
+++ b/tools/binman/etype/fit.py
@@ -62,7 +62,7 @@ class Entry_fit(Entry):
         """
         super().__init__(section, etype, node)
         self._fit = None
-        self._fit_content = defaultdict(list)
+        self._fit_images = {}
         self._fit_props = {}
 
     def ReadNode(self):
@@ -91,15 +91,18 @@ class Entry_fit(Entry):
 
             rel_path = node.path[len(base_node.path):]
             has_images = depth == 2 and rel_path.startswith('/images/')
+            if has_images:
+                entry = Entry.Create(self.section, node, etype='section')
+                entry.ReadNode()
+                self._fit_images[rel_path] = entry
+
             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.
-                    entry = Entry.Create(self.section, subnode)
-                    entry.ReadNode()
-                    self._fit_content[rel_path].append(entry)
+                    pass
                 else:
                     with fsw.add_node(subnode.name):
                         _AddNode(base_node, depth + 1, subnode)
@@ -150,13 +153,12 @@ class Entry_fit(Entry):
         Returns:
             New fdt contents (bytes)
         """
-        for path, entries in self._fit_content.items():
+        for path, image in self._fit_images.items():
             node = fdt.GetNode(path)
-            data = b''
-            for entry in entries:
-                if not entry.ObtainContents():
-                    return False
-                data += entry.GetData()
+            if not image.ObtainContents():
+                return False
+            image.Pack(0)
+            data = image.GetData()
             node.AddData('data', data)
 
         fdt.Sync(auto_resize=True)
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 8edf85c89f..ed573d8991 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -3485,5 +3485,29 @@ class TestFunctional(unittest.TestCase):
         fnode = dtb.GetNode('/images/kernel')
         self.assertNotIn('data', fnode.props)
 
+    def testFitImageSubentryAlignment(self):
+        """Test relative alignability of FIT image subentries"""
+        entry_args = {
+            'test-id': TEXT_DATA,
+        }
+        data, _, _, _ = self._DoReadFileDtb('166_fit_image_subentry_alignment.dts',
+                                            entry_args=entry_args)
+        dtb = fdt.Fdt.FromData(data)
+        dtb.Scan()
+
+        node = dtb.GetNode('/images/kernel')
+        data = dtb.GetProps(node)["data"].bytes
+        align_pad = 0x10 - (len(U_BOOT_SPL_DATA) % 0x10)
+        expected = (tools.GetBytes(0, 0x20) + U_BOOT_SPL_DATA +
+                    tools.GetBytes(0, align_pad) + U_BOOT_DATA)
+        self.assertEqual(expected, data)
+
+        node = dtb.GetNode('/images/fdt-1')
+        data = dtb.GetProps(node)["data"].bytes
+        expected = (U_BOOT_SPL_DTB_DATA + tools.GetBytes(0, 20) +
+                    tools.ToBytes(TEXT_DATA) + tools.GetBytes(0, 30) +
+                    U_BOOT_DTB_DATA)
+        self.assertEqual(expected, data)
+
 if __name__ == "__main__":
     unittest.main()
diff --git a/tools/binman/test/166_fit_image_subentry_alignment.dts b/tools/binman/test/166_fit_image_subentry_alignment.dts
new file mode 100644
index 0000000000..360cac5266
--- /dev/null
+++ b/tools/binman/test/166_fit_image_subentry_alignment.dts
@@ -0,0 +1,57 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		fit {
+			description = "test-desc";
+			#address-cells = <1>;
+
+			images {
+				kernel {
+					description = "Offset-Align Test";
+					type = "kernel";
+					arch = "arm64";
+					os = "linux";
+					compression = "none";
+					load = <00000000>;
+					entry = <00000000>;
+					u-boot-spl {
+						offset = <0x20>;
+					};
+					u-boot {
+						align = <0x10>;
+					};
+				};
+				fdt-1 {
+					description = "Pad-Before-After Test";
+					type = "flat_dt";
+					arch = "arm64";
+					compression = "none";
+					u-boot-spl-dtb {
+					};
+					text {
+						text-label = "test-id";
+						pad-before = <20>;
+						pad-after = <30>;
+					};
+					u-boot-dtb {
+					};
+				};
+			};
+
+			configurations {
+				default = "conf-1";
+				conf-1 {
+					description = "Kernel with FDT blob";
+					kernel = "kernel";
+					fdt = "fdt-1";
+				};
+			};
+		};
+	};
+};
-- 
2.28.0

[PATCH 1/3] binman: Ignore hash*, signature* nodes in sections

[Simon Glass]

Hi Alper,

On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Switch to str.startswith for matching like the FIT etype does since the
> current version doesn't ignore 'hash-1', 'hash-2', etc.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/etype/section.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index 73c5553c81..c5166a5b57 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -83,7 +83,7 @@ class Entry_section(Entry):
>
>      def _ReadEntries(self):
>          for node in self._node.subnodes:
> -            if node.name == 'hash':
> +            if node.name.startswith('hash') or node.name.startswith('signature'):
>                  continue
>              entry = Entry.Create(self, node)
>              entry.ReadNode()
> --
> 2.28.0
>

This looks like right but please add a test or update an existing one,
since code coverage is missing with this patch (binman test -T).

Regards,
Simon


On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Switch to str.startswith for matching like the FIT etype does since the
> current version doesn't ignore 'hash-1', 'hash-2', etc.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/etype/section.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index 73c5553c81..c5166a5b57 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -83,7 +83,7 @@ class Entry_section(Entry):
>
>      def _ReadEntries(self):
>          for node in self._node.subnodes:
> -            if node.name == 'hash':
> +            if node.name.startswith('hash') or node.name.startswith('signature'):
>                  continue
>              entry = Entry.Create(self, node)
>              entry.ReadNode()
> --
> 2.28.0
>

[PATCH 2/3] binman: Respect pad-before property of section subentries

[Simon Glass]

Hi Alper,

On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> Other relevant properties (pad-after, offset, size, align, align-size,
> align-end) already work since Pack() sets correct ranges for subentries'
> data (.offset, .size variables), but some padding here is necessary to
> align the data within this range to match the pad-before property.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/etype/section.py             |  2 +-
>  tools/binman/ftest.py                     |  8 +++++++
>  tools/binman/test/165_pad_in_sections.dts | 26 +++++++++++++++++++++++
>  3 files changed, 35 insertions(+), 1 deletion(-)
>  create mode 100644 tools/binman/test/165_pad_in_sections.dts
>
> diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py
> index c5166a5b57..72600b1ef3 100644
> --- a/tools/binman/etype/section.py
> +++ b/tools/binman/etype/section.py
> @@ -152,7 +152,7 @@ class Entry_section(Entry):
>          for entry in self._entries.values():
>              data = entry.GetData()
>              base = self.pad_before + (entry.offset or 0) - self._skip_at_start
> -            pad = base - len(section_data)
> +            pad = base - len(section_data) + (entry.pad_before or 0)
>              if pad > 0:
>                  section_data += tools.GetBytes(self._pad_byte, pad)
>              section_data += data
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 5f650b5f94..8edf85c89f 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -1269,6 +1269,14 @@ class TestFunctional(unittest.TestCase):
>                      U_BOOT_DATA + tools.GetBytes(ord('&'), 4))
>          self.assertEqual(expected, data)
>
> +    def testPadInSections(self):

Can you put this test last please? I'd like to keep the tests in rough
order of .dts test files.

REgards,
Simon

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

[Simon Glass]

Hi Alper,

On Tue, 25 Aug 2020 at 12:01, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> When reading subentries of each image, the FIT entry type directly
> concatenates their contents without padding them according to their
> offset, size, align, align-size, align-end, pad-before, pad-after
> properties.
>
> This patch makes sure these properties are respected by offloading this
> image-data building to the section etype, where each subnode of the
> "images" node is processed as a section. Alignments and offsets are
> respective to the beginning of each image. For example, the following
> fragment can end up having "u-boot-spl" start at 0x88 within the final
> FIT binary, while "u-boot" would then end up starting at e.g. 0x20088.
>
>         fit {
>                 description = "example";
>
>                 images {
>                         kernel-1 {
>                                 description = "U-Boot with SPL";
>                                 type = "kernel";
>                                 arch = "arm64";
>                                 os = "linux";
>                                 compression = "none";
>
>                                 u-boot-spl {
>                                 };
>                                 u-boot {
>                                         align = <0x10000>;
>                                 };
>                         };
>                 };
>         }
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
>
>  tools/binman/etype/fit.py                     | 22 +++----
>  tools/binman/ftest.py                         | 24 ++++++++
>  .../test/166_fit_image_subentry_alignment.dts | 57 +++++++++++++++++++
>  3 files changed, 93 insertions(+), 10 deletions(-)
>  create mode 100644 tools/binman/test/166_fit_image_subentry_alignment.dts

This is a nice enhancement.

A few nits below.

>
> diff --git a/tools/binman/etype/fit.py b/tools/binman/etype/fit.py
> index 75712f4409..f136a2c254 100644
> --- a/tools/binman/etype/fit.py
> +++ b/tools/binman/etype/fit.py
> @@ -62,7 +62,7 @@ class Entry_fit(Entry):
>          """
>          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?

>          self._fit_props = {}
>
>      def ReadNode(self):
> @@ -91,15 +91,18 @@ class Entry_fit(Entry):
>
>              rel_path = node.path[len(base_node.path):]
>              has_images = depth == 2 and rel_path.startswith('/images/')
> +            if has_images:
> +                entry = Entry.Create(self.section, node, etype='section')
> +                entry.ReadNode()
> +                self._fit_images[rel_path] = entry
> +
>              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.

> -                    entry = Entry.Create(self.section, subnode)
> -                    entry.ReadNode()
> -                    self._fit_content[rel_path].append(entry)
> +                    pass
>                  else:
>                      with fsw.add_node(subnode.name):
>                          _AddNode(base_node, depth + 1, subnode)
> @@ -150,13 +153,12 @@ class Entry_fit(Entry):
>          Returns:
>              New fdt contents (bytes)
>          """
> -        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.

>              node = fdt.GetNode(path)
> -            data = b''
> -            for entry in entries:
> -                if not entry.ObtainContents():
> -                    return False
> -                data += entry.GetData()
> +            if not image.ObtainContents():
> +                return False
> +            image.Pack(0)
> +            data = image.GetData()
>              node.AddData('data', data)
>
>          fdt.Sync(auto_resize=True)

[..]

Please also do check code coverage: binman test -T

Regards,
Simon

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

[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@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

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

[Alper Nebi Yasak]

On 30/08/2020 23:37, Simon Glass wrote:
> On Sun, 30 Aug 2020 at 12:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>> 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.

Section etype already does three passes over its subentries and either
returns True or raises an exception. Maybe it should return False
instead, but that breaks the 057_unknown_contents.dts test.

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

Since section does multiple passes, this appears to only add some more
data to wherever it's inserted, with no change in coverage.

I can get the exception with 'return-unknown-contents'. If I replace the
raise with 'return False', it fails in section etype's GetData(). If I
fix that (section_data += data or b''), the FIT entry returns b'' as its
entire data due to those checks and only then I can cover them with a
new test. More trouble than it's worth?

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

[Simon Glass]

Hi Alper,

On Sun, 30 Aug 2020 at 16:38, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> On 30/08/2020 23:37, Simon Glass wrote:
> > On Sun, 30 Aug 2020 at 12:17, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
> >> 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.
>
> Section etype already does three passes over its subentries and either
> returns True or raises an exception. Maybe it should return False
> instead, but that breaks the 057_unknown_contents.dts test.

We do want to detect when an entry does not get around to producing
its contents, so this is correct.

>
> > 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.
>
> Since section does multiple passes, this appears to only add some more
> data to wherever it's inserted, with no change in coverage.
>
> I can get the exception with 'return-unknown-contents'. If I replace the
> raise with 'return False', it fails in section etype's GetData(). If I
> fix that (section_data += data or b''), the FIT entry returns b'' as its
> entire data due to those checks and only then I can cover them with a
> new test. More trouble than it's worth?

OK I see. But if we later change entry_Section to be more flexible we
will get into trouble. Still, that is what test coverage is for, so I
think it is OK to not check the return value and add a comment as to
why.

Regards,
SImon