On 26/10/2020 22:22, Simon Glass wrote:
> On Mon, 19 Oct 2020 at 15:29, Alper Nebi Yasak <alpernebiya...@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.
