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

[Simon Glass]

Hi Alper,

On Mon, 19 Oct 2020 at 15:29, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> 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 at 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?

You have very sharp eyes.

The case you list seems to produce the same result before and after
this patch. But if I put the padding at the top level it does strange
things, so perhaps that's what you mean?

I've added a few test cases along these lines in v2, and one of them
certainly different behaviour. This is good actually since it shows a
simple case of what these padding changes are intended to fix.




>
> > +
> > +            # 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).

Yes, will do.

>
> >          """Test that we can handle creating two images
> >
> >          This also tests image padding.
> >

Regards,
Simon