Hi Alper, On Mon, 26 Oct 2020 at 17:20, Alper Nebi Yasak <alpernebiya...@gmail.com> wrote: > > 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.
See what you think of the above test cases - testSectionPad() and testSectionAlign() Regards, Simon