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

[Simon Glass]

Hi Alper,

On Mon, 26 Oct 2020 at 17:20, Alper Nebi Yasak <alpernebiyasak at gmail.com> wrote:
>
> On 26/10/2020 22:22, Simon Glass wrote:
> > 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:
> >>>          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.
>
> Thanks! I had actually tried to copy this to the fit etype before making
> it use the section etype directly, so I did read and think about this
> part (and Pack()) a lot to better understand how things were supposed to
> work.
>
> > 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 was trying to write a case where a pad = (-16) + (16) = 0 gets split
> into two (pad = -16, then entry.pad_before = 16) after that change,
> causing a change in padding. Still looks correct to me, but I didn't
> actually run anything so I'm probably getting something wrong.
>
> > 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.

See what you think of the above test cases - testSectionPad() and
testSectionAlign()

Regards,
Simon