| Message ID | 20220816084210.14972-3-stefan.herbrechtsmeier-oss@weidmueller.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v4,01/13] binman: Skip elf tests if python elftools is not available | expand |
Hi Stefan, On Tue, 16 Aug 2022 at 02:42, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > Disable the compressed data header of the utilities to compress and > decompress data. The header is uncommon, not supported by U-Boot and > incompatible with external compressed artifacts. > > The header was introduced as part of commit eb0f4a4cb402 ("binman: > Support replacing data in a cbfs") to allow device tree entries to be > larger than the compressed contents. > > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > > --- > > (no changes since v3) > > Changes in v3: > - Added > > tools/binman/entry.py | 2 +- > tools/binman/etype/section.py | 3 ++- > tools/binman/ftest.py | 18 +++++++++++------- > 3 files changed, 14 insertions(+), 9 deletions(-) This seems strange to me. I would expect this patch to make every call set with_header to False, then the next patch remove the parameters. That would allow later bisecting to see where a problem occurs. But at present this patch and the next seem to be mixed together. > > diff --git a/tools/binman/entry.py b/tools/binman/entry.py > index e3767aefa7..a45aeeaa59 100644 > --- a/tools/binman/entry.py > +++ b/tools/binman/entry.py > @@ -1079,7 +1079,7 @@ features to produce new behaviours. > self.uncomp_data = indata > if self.compress != 'none': > self.uncomp_size = len(indata) > - data = comp_util.compress(indata, self.compress) > + data = comp_util.compress(indata, self.compress, with_header=False) > return data > > @classmethod > diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py > index bd67238b91..0a92bf2386 100644 > --- a/tools/binman/etype/section.py > +++ b/tools/binman/etype/section.py > @@ -777,7 +777,8 @@ class Entry_section(Entry): > data = parent_data[offset:offset + child.size] > if decomp: > indata = data > - data = comp_util.decompress(indata, child.compress) > + data = comp_util.decompress(indata, child.compress, > + with_header=False) > if child.uncomp_size: > tout.info("%s: Decompressing data size %#x with algo '%s' to data size %#x" % > (child.GetPath(), len(indata), child.compress, > diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py > index 1b468d8e6d..d082442bec 100644 > --- a/tools/binman/ftest.py > +++ b/tools/binman/ftest.py > @@ -1967,7 +1967,7 @@ class TestFunctional(unittest.TestCase): > self._ResetDtbs() > > def _decompress(self, data): > - return comp_util.decompress(data, 'lz4') > + return comp_util.decompress(data, 'lz4', with_header=False) > > def testCompress(self): > """Test compression of blobs""" > @@ -4449,15 +4449,19 @@ class TestFunctional(unittest.TestCase): > rest = base[len(U_BOOT_DATA):] > > # Check compressed data > - section1 = self._decompress(rest) > - expect1 = comp_util.compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4') > - self.assertEquals(expect1, rest[:len(expect1)]) > + expect1 = comp_util.compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4', > + with_header=False) > + data1 = rest[:len(expect1)] > + section1 = self._decompress(data1) > + self.assertEquals(expect1, data1) > self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, section1) > rest1 = rest[len(expect1):] > > - section2 = self._decompress(rest1) > - expect2 = comp_util.compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4') > - self.assertEquals(expect2, rest1[:len(expect2)]) > + expect2 = comp_util.compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4', > + with_header=False) > + data2 = rest1[:len(expect2)] > + section2 = self._decompress(data2) > + self.assertEquals(expect2, data2) > self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2) > rest2 = rest1[len(expect2):] > > -- > 2.30.2 > Regards, Simon
Hi Simon, Am 16.08.2022 um 13:48 schrieb Simon Glass: > Hi Stefan, > > On Tue, 16 Aug 2022 at 02:42, Stefan Herbrechtsmeier > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: >> >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> Disable the compressed data header of the utilities to compress and >> decompress data. The header is uncommon, not supported by U-Boot and >> incompatible with external compressed artifacts. >> >> The header was introduced as part of commit eb0f4a4cb402 ("binman: >> Support replacing data in a cbfs") to allow device tree entries to be >> larger than the compressed contents. >> >> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> >> >> --- >> >> (no changes since v3) >> >> Changes in v3: >> - Added >> >> tools/binman/entry.py | 2 +- >> tools/binman/etype/section.py | 3 ++- >> tools/binman/ftest.py | 18 +++++++++++------- >> 3 files changed, 14 insertions(+), 9 deletions(-) > > This seems strange to me. > > I would expect this patch to make every call set with_header to False, > then the next patch remove the parameters. That would allow later > bisecting to see where a problem occurs. > > But at present this patch and the next seem to be mixed together. I skipped the following calls because it doesn't matter: comp_util.compress(b'', 'invalid') comp_util.decompress(b'1234', 'invalid') Do I miss a call? The cbfs_util.py file doesn't use the header.
Hi Stefan, On Tue, 16 Aug 2022 at 06:03, Stefan Herbrechtsmeier <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > > Hi Simon, > > Am 16.08.2022 um 13:48 schrieb Simon Glass: > > Hi Stefan, > > > > On Tue, 16 Aug 2022 at 02:42, Stefan Herbrechtsmeier > > <stefan.herbrechtsmeier-oss@weidmueller.com> wrote: > >> > >> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >> > >> Disable the compressed data header of the utilities to compress and > >> decompress data. The header is uncommon, not supported by U-Boot and > >> incompatible with external compressed artifacts. > >> > >> The header was introduced as part of commit eb0f4a4cb402 ("binman: > >> Support replacing data in a cbfs") to allow device tree entries to be > >> larger than the compressed contents. > >> > >> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com> > >> > >> --- > >> > >> (no changes since v3) > >> > >> Changes in v3: > >> - Added > >> > >> tools/binman/entry.py | 2 +- > >> tools/binman/etype/section.py | 3 ++- > >> tools/binman/ftest.py | 18 +++++++++++------- > >> 3 files changed, 14 insertions(+), 9 deletions(-) > > > > This seems strange to me. > > > > I would expect this patch to make every call set with_header to False, > > then the next patch remove the parameters. That would allow later > > bisecting to see where a problem occurs. > > > > But at present this patch and the next seem to be mixed together. > > I skipped the following calls because it doesn't matter: > comp_util.compress(b'', 'invalid') > comp_util.decompress(b'1234', 'invalid') I still think it is worth it, because the default is True. > > Do I miss a call? Everything should pass False in this patch I think. > > The cbfs_util.py file doesn't use the header. OK Regards, SImon
diff --git a/tools/binman/entry.py b/tools/binman/entry.py index e3767aefa7..a45aeeaa59 100644 --- a/tools/binman/entry.py +++ b/tools/binman/entry.py @@ -1079,7 +1079,7 @@ features to produce new behaviours. self.uncomp_data = indata if self.compress != 'none': self.uncomp_size = len(indata) - data = comp_util.compress(indata, self.compress) + data = comp_util.compress(indata, self.compress, with_header=False) return data @classmethod diff --git a/tools/binman/etype/section.py b/tools/binman/etype/section.py index bd67238b91..0a92bf2386 100644 --- a/tools/binman/etype/section.py +++ b/tools/binman/etype/section.py @@ -777,7 +777,8 @@ class Entry_section(Entry): data = parent_data[offset:offset + child.size] if decomp: indata = data - data = comp_util.decompress(indata, child.compress) + data = comp_util.decompress(indata, child.compress, + with_header=False) if child.uncomp_size: tout.info("%s: Decompressing data size %#x with algo '%s' to data size %#x" % (child.GetPath(), len(indata), child.compress, diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py index 1b468d8e6d..d082442bec 100644 --- a/tools/binman/ftest.py +++ b/tools/binman/ftest.py @@ -1967,7 +1967,7 @@ class TestFunctional(unittest.TestCase): self._ResetDtbs() def _decompress(self, data): - return comp_util.decompress(data, 'lz4') + return comp_util.decompress(data, 'lz4', with_header=False) def testCompress(self): """Test compression of blobs""" @@ -4449,15 +4449,19 @@ class TestFunctional(unittest.TestCase): rest = base[len(U_BOOT_DATA):] # Check compressed data - section1 = self._decompress(rest) - expect1 = comp_util.compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4') - self.assertEquals(expect1, rest[:len(expect1)]) + expect1 = comp_util.compress(COMPRESS_DATA + U_BOOT_DATA, 'lz4', + with_header=False) + data1 = rest[:len(expect1)] + section1 = self._decompress(data1) + self.assertEquals(expect1, data1) self.assertEquals(COMPRESS_DATA + U_BOOT_DATA, section1) rest1 = rest[len(expect1):] - section2 = self._decompress(rest1) - expect2 = comp_util.compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4') - self.assertEquals(expect2, rest1[:len(expect2)]) + expect2 = comp_util.compress(COMPRESS_DATA + COMPRESS_DATA, 'lz4', + with_header=False) + data2 = rest1[:len(expect2)] + section2 = self._decompress(data2) + self.assertEquals(expect2, data2) self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2) rest2 = rest1[len(expect2):]