On 20/04/2022 00:54, Simon Glass wrote: > On Sun, 27 Mar 2022 at 09:32, Alper Nebi Yasak <alpernebiya...@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 <alpernebiya...@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 <s...@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 >>