Re: [PATCH 10/25] binman: Move section-building code into a function

On 19/10/2020 05:41, Simon Glass wrote:

Create a new _BuildSectionData() to hold the code that is now in GetData(), so that it is clearly separated from entry.GetData() base function.

Separate out the 'pad-before' processing to make this easier to understand.

Unfortunately this breaks the testDual test. Rather than squash several patches into an un-reviewable glob, disable the test for now.

Signed-off-by: Simon Glass sjg@chromium.org

tools/binman/etype/section.py | 35 +++++++++++++++++++++++++++++------ tools/binman/ftest.py | 3 ++- 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index 9222042f5d8..d05adf00274 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -144,24 +144,47 @@ class Entry_section(Entry): def ObtainContents(self): return self.GetEntryContents()

• def GetData(self):

• def _BuildSectionData(self):

•    """Build the contents of a section
  

•    This places all entries at the right place, dealing with padding before
  

•    and after entries. It does not do padding for the section itself (the
  

•    pad-before and pad-after properties in the section items) since that is
  

•    handled by the parent section.
  

•    Returns:
  

•        Contents of the section (bytes)
  

•    """
     section_data = b''
  
     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) + (entry.pad_before or 0)
  

•        # Handle empty space before the entry
  

•        pad = (entry.offset or 0) - self._skip_at_start - len(section_data)
         if pad > 0:
             section_data += tools.GetBytes(self._pad_byte, pad)
  

•        # Handle padding before the entry
  

•        if entry.pad_before:
  

•            section_data += tools.GetBytes(self._pad_byte, entry.pad_before)
  

Consider this fragment:

section { skip-at-start = <16>;

blob { pad-before = <16>; filename = "foo"; } }

Is this invalid as 'blob' isn't offset > skip-at-start? This wouldn't be padded in the earlier code, but would be padded after this (assuming it doesn't error out) -- was that a bug or undefined behaviour or something?

•        # Add in the actual entry data
         section_data += data
  

•        # Handle padding after the entry
  

•        if entry.pad_after:
  

•            section_data += tools.GetBytes(self._pad_byte, entry.pad_after)
  

•    if self.size:
  

•        pad = self.size - len(section_data)
  

•        if pad > 0:
  

•            section_data += tools.GetBytes(self._pad_byte, pad)
  

•        section_data += tools.GetBytes(self._pad_byte,
  

•                                       self.size - len(section_data))
     self.Detail('GetData: %d entries, total size %#x' %
                 (len(self._entries), len(section_data)))
     return self.CompressData(section_data)
  
  

• def GetData(self):

•    return self._BuildSectionData()
  

• def GetOffsets(self): """Handle entries that want to set the offset/size of other entries

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index e265941a392..3c6eb7f6736 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -706,7 +706,8 @@ class TestFunctional(unittest.TestCase): """Test a simple binman run with debugging enabled""" self._DoTestFile('005_simple.dts', debug=True)

• def testDual(self):

• # Disable for now until padding of images is supported
• def xtestDual(self):

I think you could use @unittest.skip() here, but perhaps it's not worth doing a v2 for that (especially since you're re-enabling it later in the series).

     """Test that we can handle creating two images

     This also tests image padding.