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

On 26/10/2020 22:22, Simon Glass wrote:

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