On 20/04/2022 00:54, Simon Glass wrote:
On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak alpernebiyasak@gmail.com wrote:
Binman interfaces allow attempts to replace any entry in the image with arbitrary data. When trying to replace sections, the changes in the section entry's data are not propagated to its child entries. This, combined with how sections rebuild their contents from its children, eventually causes the replaced contents to be silently overwritten by rebuilt contents equivalent to the original data.
Add a simple test for replacing a section that is currently failing due to this behaviour, and mark it as an expected failure. Also, raise an error when replacing a section instead of silently pretending it was replaced.
Signed-off-by: Alper Nebi Yasak alpernebiyasak@gmail.com
tools/binman/etype/section.py | 3 +++ tools/binman/ftest.py | 9 ++++++++ .../test/234_replace_section_simple.dts | 23 +++++++++++++++++++ 3 files changed, 35 insertions(+) create mode 100644 tools/binman/test/234_replace_section_simple.dts
Reviewed-by: Simon Glass sjg@chromium.org
Is it not better to assertRaises() and check that the error message is as expected?
IMO, that would imply the tested 'binman replace' call should raise an error instead of replacing the data. The test should work as-is, and when section replacement is fixed we can just remove the expectedFailure line to be strict about it.
diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index ccac658c1831..bd67238b9199 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -788,6 +788,9 @@ def ReadChildData(self, child, decomp=True, alt_format=None): data = new_data return data
- def WriteData(self, data, decomp=True):
self.Raise("Replacing sections is not implemented yet")- def WriteChildData(self, child): return True
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 43bec4a88841..058651cc18a0 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -5641,6 +5641,15 @@ def testReplaceFitSubentryLeafSmallerSize(self): self.assertIsNotNone(path) self.assertEqual(expected_fdtmap, fdtmap)
- @unittest.expectedFailure
- def testReplaceSectionSimple(self):
"""Test replacing a simple section with arbitrary data"""new_data = b'w' * len(COMPRESS_DATA + U_BOOT_DATA)data, expected_fdtmap, _ = self._RunReplaceCmd('section', new_data,dts='234_replace_section_simple.dts')self.assertEqual(new_data, data)if __name__ == "__main__": unittest.main() diff --git a/tools/binman/test/234_replace_section_simple.dts b/tools/binman/test/234_replace_section_simple.dts new file mode 100644 index 000000000000..c9d5c3285615 --- /dev/null +++ b/tools/binman/test/234_replace_section_simple.dts @@ -0,0 +1,23 @@ +// SPDX-License-Identifier: GPL-2.0+ +/dts-v1/;
+/ {
binman {allow-repack;u-boot-dtb {};section {blob {filename = "compress";};u-boot {};};fdtmap {};};+};
2.35.1