Re: [PATCH 7/7] binman: Refuse to replace sections for now

[Alper Nebi Yasak <alpernebiyasak@gmail.com>]

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