All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available
@ 2022-08-08 10:51 Stefan Herbrechtsmeier
  2022-08-08 10:51 ` [PATCH v2 02/10] binman: Make compressed data header optional Stefan Herbrechtsmeier
                   ` (9 more replies)
  0 siblings, 10 replies; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Skip tests which requires python elftools if the tool is not available.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Added

 tools/binman/elf_test.py | 14 ++++++++++++++
 tools/binman/ftest.py    | 18 ++++++++++++++++++
 2 files changed, 32 insertions(+)

diff --git a/tools/binman/elf_test.py b/tools/binman/elf_test.py
index 5a51c64cfe..75b867c2be 100644
--- a/tools/binman/elf_test.py
+++ b/tools/binman/elf_test.py
@@ -122,6 +122,8 @@ class TestElf(unittest.TestCase):
 
     def testOutsideFile(self):
         """Test a symbol which extends outside the entry area is detected"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         entry = FakeEntry(10)
         section = FakeSection()
         elf_fname = self.ElfTestFile('u_boot_binman_syms')
@@ -147,6 +149,8 @@ class TestElf(unittest.TestCase):
         Only 32 and 64 bits are supported, since we need to store an offset
         into the image.
         """
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         entry = FakeEntry(10)
         section = FakeSection()
         elf_fname =self.ElfTestFile('u_boot_binman_syms_size')
@@ -161,6 +165,8 @@ class TestElf(unittest.TestCase):
         This should produce -1 values for all thress symbols, taking up the
         first 16 bytes of the image.
         """
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         entry = FakeEntry(28)
         section = FakeSection(sym_value=None)
         elf_fname = self.ElfTestFile('u_boot_binman_syms')
@@ -172,6 +178,8 @@ class TestElf(unittest.TestCase):
 
     def testDebug(self):
         """Check that enabling debug in the elf module produced debug output"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         try:
             tout.init(tout.DEBUG)
             entry = FakeEntry(24)
@@ -281,6 +289,8 @@ class TestElf(unittest.TestCase):
 
     def test_read_segments_bad_data(self):
         """Test for read_loadable_segments() with an invalid ELF file"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         fname = self.ElfTestFile('embed_data')
         with self.assertRaises(ValueError) as e:
             elf.read_loadable_segments(tools.get_bytes(100, 100))
@@ -288,6 +298,8 @@ class TestElf(unittest.TestCase):
 
     def test_get_file_offset(self):
         """Test GetFileOffset() gives the correct file offset for a symbol"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         fname = self.ElfTestFile('embed_data')
         syms = elf.GetSymbols(fname, ['embed'])
         addr = syms['embed'].address
@@ -314,6 +326,8 @@ class TestElf(unittest.TestCase):
 
     def test_get_symbol_from_address(self):
         """Test GetSymbolFromAddress()"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         fname = self.ElfTestFile('elf_sections')
         sym_name = 'calculate'
         syms = elf.GetSymbols(fname, [sym_name])
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index fa1f421c05..da9aa9e679 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -4807,6 +4807,8 @@ class TestFunctional(unittest.TestCase):
 
     def testUpdateFdtInElf(self):
         """Test that we can update the devicetree in an ELF file"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         infile = elf_fname = self.ElfTestFile('u_boot_binman_embed')
         outfile = os.path.join(self._indir, 'u-boot.out')
         begin_sym = 'dtb_embed_begin'
@@ -4858,6 +4860,8 @@ class TestFunctional(unittest.TestCase):
 
     def testUpdateFdtInElfNoSyms(self):
         """Test that missing symbols are detected with --update-fdt-in-elf"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         infile = elf_fname = self.ElfTestFile('u_boot_binman_embed')
         outfile = ''
         begin_sym = 'wrong_begin'
@@ -4871,6 +4875,8 @@ class TestFunctional(unittest.TestCase):
 
     def testUpdateFdtInElfTooSmall(self):
         """Test that an over-large dtb is detected with --update-fdt-in-elf"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         infile = elf_fname = self.ElfTestFile('u_boot_binman_embed_sm')
         outfile = os.path.join(self._indir, 'u-boot.out')
         begin_sym = 'dtb_embed_begin'
@@ -5344,6 +5350,8 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testFitSplitElf(self):
         """Test an image with an FIT with an split-elf operation"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         entry_args = {
             'of-list': 'test-fdt1 test-fdt2',
             'default-dt': 'test-fdt2',
@@ -5421,6 +5429,8 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testFitSplitElfBadElf(self):
         """Test a FIT split-elf operation with an invalid ELF file"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         TestFunctional._MakeInputFile('bad.elf', tools.get_bytes(100, 100))
         entry_args = {
             'of-list': 'test-fdt1 test-fdt2',
@@ -5440,6 +5450,8 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testFitSplitElfBadDirective(self):
         """Test a FIT split-elf invalid fit,xxx directive in an image node"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         err = self._check_bad_fit('227_fit_bad_dir.dts')
         self.assertIn(
             "Node '/binman/fit': subnode 'images/@atf-SEQ': Unknown directive 'fit,something'",
@@ -5447,6 +5459,8 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testFitSplitElfBadDirectiveConfig(self):
         """Test a FIT split-elf with invalid fit,xxx directive in config"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         err = self._check_bad_fit('228_fit_bad_dir_config.dts')
         self.assertEqual(
             "Node '/binman/fit': subnode 'configurations/@config-SEQ': Unknown directive 'fit,config'",
@@ -5470,6 +5484,8 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testFitSplitElfMissing(self):
         """Test an split-elf FIT with a missing ELF file"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         err = self.checkFitSplitElf(allow_missing=True)
         self.assertRegex(
             err,
@@ -5477,6 +5493,8 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testFitSplitElfFaked(self):
         """Test an split-elf FIT with faked ELF file"""
+        if not elf.ELF_TOOLS:
+            self.skipTest('Python elftools not available')
         err = self.checkFitSplitElf(allow_missing=True, allow_fake_blobs=True)
         self.assertRegex(
             err,
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 02/10] binman: Make compressed data header optional
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 03/10] binman: Simplify comp_util class Stefan Herbrechtsmeier
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Move the compressed data header handling into the dtb blob entry class
and make it optional. The header is uncommon, not supported by U-Boot
and incompatible with external compressed artifacts.

If needed the header could be enabled with the following
attribute beside the compress attribute:
  prepend = "length";

The header was introduced as part of commit eb0f4a4cb402 ("binman:
Support replacing data in a cbfs") to allow device tree entries to be
larger that the compressed contents. Regarding the commit "this is
necessary to cope with a compressed device tree being updated in such a
way that it shrinks after the entry size is already set (an obscure
case)". This case need to be fixed without influence any compressed data
by itself.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Reworked to make the feature optional.

 tools/binman/cbfs_util.py                     |  8 ++---
 tools/binman/comp_util.py                     | 11 ++----
 tools/binman/entries.rst                      |  4 +++
 tools/binman/entry.py                         |  2 +-
 tools/binman/etype/blob_dtb.py                | 21 ++++++++++++
 tools/binman/ftest.py                         | 34 ++++++++++++++++---
 .../test/235_compress_prepend_length_dtb.dts  | 17 ++++++++++
 7 files changed, 78 insertions(+), 19 deletions(-)
 create mode 100644 tools/binman/test/235_compress_prepend_length_dtb.dts

diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py
index 9cad03886f..a1836f4ad3 100644
--- a/tools/binman/cbfs_util.py
+++ b/tools/binman/cbfs_util.py
@@ -241,9 +241,9 @@ class CbfsFile(object):
         """Handle decompressing data if necessary"""
         indata = self.data
         if self.compress == COMPRESS_LZ4:
-            data = comp_util.decompress(indata, 'lz4', with_header=False)
+            data = comp_util.decompress(indata, 'lz4')
         elif self.compress == COMPRESS_LZMA:
-            data = comp_util.decompress(indata, 'lzma', with_header=False)
+            data = comp_util.decompress(indata, 'lzma')
         else:
             data = indata
         self.memlen = len(data)
@@ -362,9 +362,9 @@ class CbfsFile(object):
         elif self.ftype == TYPE_RAW:
             orig_data = data
             if self.compress == COMPRESS_LZ4:
-                data = comp_util.compress(orig_data, 'lz4', with_header=False)
+                data = comp_util.compress(orig_data, 'lz4')
             elif self.compress == COMPRESS_LZMA:
-                data = comp_util.compress(orig_data, 'lzma', with_header=False)
+                data = comp_util.compress(orig_data, 'lzma')
             self.memlen = len(orig_data)
             self.data_len = len(data)
             attr = struct.pack(ATTR_COMPRESSION_FORMAT,
diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
index dc76adab35..269bbf7975 100644
--- a/tools/binman/comp_util.py
+++ b/tools/binman/comp_util.py
@@ -3,7 +3,6 @@
 #
 """Utilities to compress and decompress data"""
 
-import struct
 import tempfile
 
 from binman import bintool
@@ -16,7 +15,7 @@ LZMA_ALONE = bintool.Bintool.create('lzma_alone')
 HAVE_LZMA_ALONE = LZMA_ALONE.is_present()
 
 
-def compress(indata, algo, with_header=True):
+def compress(indata, algo):
     """Compress some data using a given algorithm
 
     Note that for lzma this uses an old version of the algorithm, not that
@@ -41,12 +40,9 @@ def compress(indata, algo, with_header=True):
         data = LZMA_ALONE.compress(indata)
     else:
         raise ValueError("Unknown algorithm '%s'" % algo)
-    if with_header:
-        hdr = struct.pack('<I', len(data))
-        data = hdr + data
     return data
 
-def decompress(indata, algo, with_header=True):
+def decompress(indata, algo):
     """Decompress some data using a given algorithm
 
     Note that for lzma this uses an old version of the algorithm, not that
@@ -64,9 +60,6 @@ def decompress(indata, algo, with_header=True):
     """
     if algo == 'none':
         return indata
-    if with_header:
-        data_len = struct.unpack('<I', indata[:4])[0]
-        indata = indata[4:4 + data_len]
     if algo == 'lz4':
         data = LZ4.decompress(indata)
     elif algo == 'lzma':
diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
index ae4305c99e..8e722426d3 100644
--- a/tools/binman/entries.rst
+++ b/tools/binman/entries.rst
@@ -208,6 +208,10 @@ This is a blob containing a device tree. The contents of the blob are
 obtained from the list of available device-tree files, managed by the
 'state' module.
 
+Additional Properties / Entry arguments:
+    - prepend: Header type to use:
+        none: No header
+        length: 32-bit length header
 
 
 Entry: blob-ext: Externally built binary blob
diff --git a/tools/binman/entry.py b/tools/binman/entry.py
index a07a588864..8cbfd43af9 100644
--- a/tools/binman/entry.py
+++ b/tools/binman/entry.py
@@ -1069,7 +1069,7 @@ features to produce new behaviours.
             indata: Data to compress
 
         Returns:
-            Compressed data (first word is the compressed size)
+            Compressed data
         """
         self.uncomp_data = indata
         if self.compress != 'none':
diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py
index 4159e3032a..652b8abd8f 100644
--- a/tools/binman/etype/blob_dtb.py
+++ b/tools/binman/etype/blob_dtb.py
@@ -7,6 +7,8 @@
 
 from binman.entry import Entry
 from binman.etype.blob import Entry_blob
+from dtoc import fdt_util
+import struct
 
 # This is imported if needed
 state = None
@@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob):
     This is a blob containing a device tree. The contents of the blob are
     obtained from the list of available device-tree files, managed by the
     'state' module.
+
+    Additional attributes:
+        prepend: Header used (e.g. 'length'), 'none' if none
     """
     def __init__(self, section, etype, node):
         # Put this here to allow entry-docs and help to work without libfdt
@@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob):
 
         super().__init__(section, etype, node)
 
+        self.prepend = 'none'
+
+    def ReadNode(self):
+        super().ReadNode()
+        self.prepend = fdt_util.GetString(self._node, 'prepend', 'none')
+
     def ObtainContents(self):
         """Get the device-tree from the list held by the 'state' module"""
         self._filename = self.GetDefaultFilename()
@@ -35,6 +46,9 @@ class Entry_blob_dtb(Entry_blob):
         """Re-read the DTB contents so that we get any calculated properties"""
         _, indata = state.GetFdtContents(self.GetFdtEtype())
         data = self.CompressData(indata)
+        if self.prepend == 'length':
+            hdr = struct.pack('<I', len(data))
+            data = hdr + data
         return self.ProcessContentsUpdate(data)
 
     def GetFdtEtype(self):
@@ -50,6 +64,13 @@ class Entry_blob_dtb(Entry_blob):
         fname = self.GetDefaultFilename()
         return {self.GetFdtEtype(): [self, fname]}
 
+    def ReadData(self, decomp=True, alt_format=None):
+        data = super().ReadData(decomp, alt_format)
+        if self.prepend == 'length':
+            data_len = struct.unpack('<I', data[:4])[0]
+            data = data[4:4 + data_len]
+        return data
+
     def WriteData(self, data, decomp=True):
         ok = super().WriteData(data, decomp)
 
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index da9aa9e679..057b4e28b7 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -2536,6 +2536,28 @@ class TestFunctional(unittest.TestCase):
             }
         self.assertEqual(expected, props)
 
+    def testCompressPrependLengthDtb(self):
+        """Test that compress of device-tree files with length header is
+        supported
+        """
+        data = self.data = self._DoReadFileRealDtb('235_compress_prepend_length_dtb.dts')
+        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
+        dtb_data = data[len(U_BOOT_DATA):]
+        comp_data_len = struct.unpack('<I', dtb_data[:4])[0]
+        comp_data = dtb_data[4:4 + comp_data_len]
+        orig = self._decompress(comp_data)
+        dtb = fdt.Fdt.FromData(orig)
+        dtb.Scan()
+        props = self._GetPropTree(dtb, ['size', 'uncomp-size'])
+        expected = {
+            'u-boot:size': len(U_BOOT_DATA),
+            'u-boot-dtb:uncomp-size': len(orig),
+            'u-boot-dtb:size': len(dtb_data),
+            'size': len(data),
+            }
+        self.assertEqual(expected, props)
+
+
     def testCbfsUpdateFdt(self):
         """Test that we can update the device tree with CBFS offset/size info"""
         self._CheckLz4()
@@ -2856,7 +2878,7 @@ class TestFunctional(unittest.TestCase):
     def testExtractCbfsRaw(self):
         """Test extracting CBFS compressed data without decompressing it"""
         data = self._RunExtractCmd('section/cbfs/u-boot-dtb', decomp=False)
-        dtb = comp_util.decompress(data, 'lzma', with_header=False)
+        dtb = comp_util.decompress(data, 'lzma')
         self.assertEqual(EXTRACT_DTB_SIZE, len(dtb))
 
     def testExtractBadEntry(self):
@@ -4427,15 +4449,17 @@ 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)])
+        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)])
+        data2 = rest1[:len(expect2)]
+        section2 = self._decompress(data2)
+        self.assertEquals(expect2, data2)
         self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2)
         rest2 = rest1[len(expect2):]
 
diff --git a/tools/binman/test/235_compress_prepend_length_dtb.dts b/tools/binman/test/235_compress_prepend_length_dtb.dts
new file mode 100644
index 0000000000..a5abc60477
--- /dev/null
+++ b/tools/binman/test/235_compress_prepend_length_dtb.dts
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/dts-v1/;
+
+/ {
+	#address-cells = <1>;
+	#size-cells = <1>;
+
+	binman {
+		u-boot {
+		};
+		u-boot-dtb {
+			compress = "lz4";
+			prepend = "length";
+		};
+	};
+};
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 03/10] binman: Simplify comp_util class
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
  2022-08-08 10:51 ` [PATCH v2 02/10] binman: Make compressed data header optional Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 04/10] binman: Add compression tests Stefan Herbrechtsmeier
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Simplify the comp_util class by remove duplicate code as preparation for
additional compress algorithms.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
---

(no changes since v1)

 tools/binman/cbfs_util_test.py |  2 +-
 tools/binman/comp_util.py      | 46 +++++++++++++++++++++++-----------
 tools/binman/ftest.py          |  2 +-
 3 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/tools/binman/cbfs_util_test.py b/tools/binman/cbfs_util_test.py
index f86b295149..44ebd04278 100755
--- a/tools/binman/cbfs_util_test.py
+++ b/tools/binman/cbfs_util_test.py
@@ -50,7 +50,7 @@ class TestCbfs(unittest.TestCase):
         cls.cbfstool = bintool.Bintool.create('cbfstool')
         cls.have_cbfstool = cls.cbfstool.is_present()
 
-        cls.have_lz4 = comp_util.HAVE_LZ4
+        cls.have_lz4 = comp_util.is_present('lz4')
 
     @classmethod
     def tearDownClass(cls):
diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
index 269bbf7975..faa08a7f8a 100644
--- a/tools/binman/comp_util.py
+++ b/tools/binman/comp_util.py
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+
 # Copyright 2022 Google LLC
+# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
+# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
 #
 """Utilities to compress and decompress data"""
 
@@ -8,12 +10,23 @@ import tempfile
 from binman import bintool
 from patman import tools
 
-LZ4 = bintool.Bintool.create('lz4')
-HAVE_LZ4 = LZ4.is_present()
+# Supported compressions
+COMPRESSIONS = ['lz4', 'lzma']
 
-LZMA_ALONE = bintool.Bintool.create('lzma_alone')
-HAVE_LZMA_ALONE = LZMA_ALONE.is_present()
+bintools = {}
 
+def _get_tool_name(algo):
+    names = {'lzma': 'lzma_alone'}
+    return names.get(algo, algo)
+
+def _get_tool(algo):
+    global bintools
+    name = _get_tool_name(algo)
+    tool = bintools.get(name)
+    if not tool:
+        tool = bintool.Bintool.create(name)
+        bintools[name] = tool
+    return tool
 
 def compress(indata, algo):
     """Compress some data using a given algorithm
@@ -33,13 +46,12 @@ def compress(indata, algo):
     """
     if algo == 'none':
         return indata
-    if algo == 'lz4':
-        data = LZ4.compress(indata)
-    # cbfstool uses a very old version of lzma
-    elif algo == 'lzma':
-        data = LZMA_ALONE.compress(indata)
-    else:
+    if algo not in COMPRESSIONS:
         raise ValueError("Unknown algorithm '%s'" % algo)
+
+    tool = _get_tool(algo)
+    data = tool.compress(indata)
+
     return data
 
 def decompress(indata, algo):
@@ -60,10 +72,14 @@ def decompress(indata, algo):
     """
     if algo == 'none':
         return indata
-    if algo == 'lz4':
-        data = LZ4.decompress(indata)
-    elif algo == 'lzma':
-        data = LZMA_ALONE.decompress(indata)
-    else:
+    if algo not in COMPRESSIONS:
         raise ValueError("Unknown algorithm '%s'" % algo)
+
+    tool = _get_tool(algo)
+    data = tool.decompress(indata)
+
     return data
+
+def is_present(algo):
+     tool = _get_tool(algo)
+     return tool.is_present()
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 057b4e28b7..96c15cff77 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -212,7 +212,7 @@ class TestFunctional(unittest.TestCase):
         TestFunctional._MakeInputFile('tee.elf',
             tools.read_file(cls.ElfTestFile('elf_sections')))
 
-        cls.have_lz4 = comp_util.HAVE_LZ4
+        cls.have_lz4 = comp_util.is_present('lz4')
 
     @classmethod
     def tearDownClass(cls):
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 04/10] binman: Add compression tests
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
  2022-08-08 10:51 ` [PATCH v2 02/10] binman: Make compressed data header optional Stefan Herbrechtsmeier
  2022-08-08 10:51 ` [PATCH v2 03/10] binman: Simplify comp_util class Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 05/10] binman: Add BintoolPacker class to bintool Stefan Herbrechtsmeier
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add common test functions to test all supported compressions.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---
Instead of the for loop it is possible to use Parameterized [1] testing.

[1] https://github.com/wolever/parameterized

Changes in v2:
- Added

 tools/binman/ftest.py | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index 96c15cff77..c9b67c48d6 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5248,6 +5248,30 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
             comp_util.decompress(b'1234', 'invalid')
         self.assertIn("Unknown algorithm 'invalid'", str(e.exception))
 
+    def testCompressions(self):
+        """Test compression algorithms"""
+        for algo in comp_util.COMPRESSIONS:
+            data = comp_util.compress(COMPRESS_DATA, algo)
+            self.assertNotEqual(COMPRESS_DATA, data)
+            orig = comp_util.decompress(data, algo)
+            self.assertEquals(COMPRESS_DATA, orig)
+
+    def testVersions(self):
+        """Test tool version of compression algorithms"""
+        for algo in comp_util.COMPRESSIONS:
+            tool = comp_util._get_tool(algo)
+            version = tool.version()
+            print('%s - %s' % (algo, version))
+            self.assertRegex(version, '^v?[0-9]+[0-9.]*')
+
+    def testPadding(self):
+        """Test padding of compression algorithms"""
+        for algo in comp_util.COMPRESSIONS:
+            data = comp_util.compress(COMPRESS_DATA, algo)
+            data = data + bytes([0]) * 64
+            orig = comp_util.decompress(data, algo)
+            self.assertEquals(COMPRESS_DATA, orig)
+
     def testBintoolDocs(self):
         """Test for creation of bintool documentation"""
         with test_util.capture_sys_output() as (stdout, stderr):
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 05/10] binman: Add BintoolPacker class to bintool
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
                   ` (2 preceding siblings ...)
  2022-08-08 10:51 ` [PATCH v2 04/10] binman: Add compression tests Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 06/10] binman: Add bzip2 bintool Stefan Herbrechtsmeier
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add a bintools base class for packers which compression / decompression
entry contents.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Added

 tools/binman/bintool.py | 94 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 94 insertions(+)

diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
index 8435b29749..1d22962790 100644
--- a/tools/binman/bintool.py
+++ b/tools/binman/bintool.py
@@ -1,5 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0+
 # Copyright 2022 Google LLC
+# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
+# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
 #
 """Base class for all bintools
 
@@ -464,3 +466,95 @@ binaries. It is fairly easy to create new bintools. Just add a new file to the
             str: Version string for this bintool
         """
         return 'unknown'
+
+class BintoolPacker(Bintool):
+    """Tool which compression / decompression entry contents
+
+    This is a bintools base class for compression / decompression packer
+    """
+    def __init__(self, name, compression=None, compress_args=None,
+                 decompress_args=None, fetch_package=None,
+                 version_regex=r'(v[0-9.]+)'):
+        desc = '%s compression' % (compression if compression else name)
+        super().__init__(name, desc)
+        if compress_args is None:
+            compress_args = ['--compress']
+        self.compress_args = compress_args
+        if decompress_args is None:
+            decompress_args = ['--decompress']
+        self.decompress_args = decompress_args
+        if fetch_package is None:
+            fetch_package = name
+        self.fetch_package = fetch_package
+        self.version_regex = version_regex
+
+    def compress(self, indata):
+        """Compress data
+
+        Args:
+            indata (bytes): Data to compress
+
+        Returns:
+            bytes: Compressed data
+        """
+        with tempfile.NamedTemporaryFile(prefix='comp.tmp',
+                                         dir=tools.get_output_dir()) as tmp:
+            tools.write_file(tmp.name, indata)
+            args = self.compress_args + ['--stdout', tmp.name]
+            return self.run_cmd(*args, binary=True)
+
+    def decompress(self, indata):
+        """Decompress data
+
+        Args:
+            indata (bytes): Data to decompress
+
+        Returns:
+            bytes: Decompressed data
+        """
+        with tempfile.NamedTemporaryFile(prefix='decomp.tmp',
+                                         dir=tools.get_output_dir()) as inf:
+            tools.write_file(inf.name, indata)
+            args = self.decompress_args + ['--stdout', inf.name]
+            return self.run_cmd(*args, binary=True)
+
+    def fetch(self, method):
+        """Fetch handler
+
+        This installs the gzip package using the apt utility.
+
+        Args:
+            method (FETCH_...): Method to use
+
+        Returns:
+            True if the file was fetched and now installed, None if a method
+            other than FETCH_BIN was requested
+
+        Raises:
+            Valuerror: Fetching could not be completed
+        """
+        if method != FETCH_BIN:
+            return None
+        pkg = self.fetch_package if self.fetch_package else self.name
+        return self.apt_install(pkg)
+
+    def version(self):
+        """Version handler
+
+        Returns:
+            str: Version number
+        """
+        import re
+
+        result = self.run_cmd_result('-V')
+        if not result:
+            return super().version()
+
+        out = result.stdout.strip()
+        if not out:
+            out = result.stderr.strip()
+        if not out:
+            return super().version()
+
+        m_version = re.search(self.version_regex, out)
+        return m_version.group(1) if m_version else out
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 06/10] binman: Add bzip2 bintool
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
                   ` (3 preceding siblings ...)
  2022-08-08 10:51 ` [PATCH v2 05/10] binman: Add BintoolPacker class to bintool Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 07/10] binman: Add gzip bintool Stefan Herbrechtsmeier
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add bzip2 bintool to binman to support on-the-fly compression.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Added

 tools/binman/btool/bzip2.py | 30 ++++++++++++++++++++++++++++++
 tools/binman/comp_util.py   | 14 +++++++-------
 2 files changed, 37 insertions(+), 7 deletions(-)
 create mode 100644 tools/binman/btool/bzip2.py

diff --git a/tools/binman/btool/bzip2.py b/tools/binman/btool/bzip2.py
new file mode 100644
index 0000000000..9be87a621f
--- /dev/null
+++ b/tools/binman/btool/bzip2.py
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
+# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
+#
+"""Bintool implementation for bzip2
+
+bzip2 allows compression and decompression of files.
+
+Documentation is available via::
+
+   man bzip2
+"""
+
+from binman import bintool
+
+# pylint: disable=C0103
+class Bintoolbzip2(bintool.BintoolPacker):
+    """Compression/decompression using the bzip2 algorithm
+
+    This bintool supports running `bzip2` to compress and decompress data, as
+    used by binman.
+
+    It is also possible to fetch the tool, which uses `apt` to install it.
+
+    Documentation is available via::
+
+        man bzip2
+    """
+    def __init__(self, name):
+        super().__init__(name, version_regex=r'bzip2.*Version ([0-9.]+)')
diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
index faa08a7f8a..fa75911e58 100644
--- a/tools/binman/comp_util.py
+++ b/tools/binman/comp_util.py
@@ -11,7 +11,7 @@ from binman import bintool
 from patman import tools
 
 # Supported compressions
-COMPRESSIONS = ['lz4', 'lzma']
+COMPRESSIONS = ['bzip2', 'lz4', 'lzma']
 
 bintools = {}
 
@@ -34,12 +34,12 @@ def compress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'lz4' and 'lzma_alone' tools. It also requires an output
-    directory to be previously set up, by calling PrepareOutputDir().
+    This requires 'bzip2', 'lz4' and 'lzma_alone' tools. It also requires an
+    output directory to be previously set up, by calling PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to compress
-        algo (str): Algorithm to use ('none', 'lz4' or 'lzma')
+        algo (str): Algorithm to use ('none', 'bzip2', 'lz4' or 'lzma')
 
     Returns:
         bytes: Compressed data
@@ -60,12 +60,12 @@ def decompress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'lz4' and 'lzma_alone' tools. It also requires an output
-    directory to be previously set up, by calling PrepareOutputDir().
+    This requires 'bzip2', 'lz4' and 'lzma_alone' tools. It also requires an
+    output directory to be previously set up, by calling PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to decompress
-        algo (str): Algorithm to use ('none', 'lz4' or 'lzma')
+        algo (str): Algorithm to use ('none', 'bzip2', 'lz4' or 'lzma')
 
     Returns:
         (bytes) Compressed data
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 07/10] binman: Add gzip bintool
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
                   ` (4 preceding siblings ...)
  2022-08-08 10:51 ` [PATCH v2 06/10] binman: Add bzip2 bintool Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 08/10] binman: Add lzop bintool Stefan Herbrechtsmeier
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add gzip bintool to binman to support on-the-fly compression of Linux
kernel images and FPGA bitstreams. The SPL basic fitImage implementation
supports only gzip decompression.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Added

 tools/binman/btool/gzip.py | 31 +++++++++++++++++++++++++++++++
 tools/binman/comp_util.py  | 16 +++++++++-------
 2 files changed, 40 insertions(+), 7 deletions(-)
 create mode 100644 tools/binman/btool/gzip.py

diff --git a/tools/binman/btool/gzip.py b/tools/binman/btool/gzip.py
new file mode 100644
index 0000000000..0d75028120
--- /dev/null
+++ b/tools/binman/btool/gzip.py
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
+# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
+#
+"""Bintool implementation for gzip
+
+gzip allows compression and decompression of files.
+
+Documentation is available via::
+
+   man gzip
+"""
+
+from binman import bintool
+
+# pylint: disable=C0103
+class Bintoolgzip(bintool.BintoolPacker):
+    """Compression/decompression using the gzip algorithm
+
+    This bintool supports running `gzip` to compress and decompress data, as
+    used by binman.
+
+    It is also possible to fetch the tool, which uses `apt` to install it.
+
+    Documentation is available via::
+
+        man gzip
+    """
+    def __init__(self, name):
+        super().__init__(name, compress_args=[],
+                         version_regex=r'gzip ([0-9.]+)')
diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
index fa75911e58..4fbd80e2ff 100644
--- a/tools/binman/comp_util.py
+++ b/tools/binman/comp_util.py
@@ -11,7 +11,7 @@ from binman import bintool
 from patman import tools
 
 # Supported compressions
-COMPRESSIONS = ['bzip2', 'lz4', 'lzma']
+COMPRESSIONS = ['bzip2', 'gzip', 'lz4', 'lzma']
 
 bintools = {}
 
@@ -34,12 +34,13 @@ def compress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'lz4' and 'lzma_alone' tools. It also requires an
-    output directory to be previously set up, by calling PrepareOutputDir().
+    This requires 'bzip2', 'gzip', 'lz4' and 'lzma_alone' tools. It also
+    requires an output directory to be previously set up, by calling
+    PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to compress
-        algo (str): Algorithm to use ('none', 'bzip2', 'lz4' or 'lzma')
+        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4' or 'lzma')
 
     Returns:
         bytes: Compressed data
@@ -60,12 +61,13 @@ def decompress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'lz4' and 'lzma_alone' tools. It also requires an
-    output directory to be previously set up, by calling PrepareOutputDir().
+    This requires 'bzip2', 'gzip', 'lz4' and 'lzma_alone' tools. It also
+    requires an output directory to be previously set up, by calling
+    PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to decompress
-        algo (str): Algorithm to use ('none', 'bzip2', 'lz4' or 'lzma')
+        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4' or 'lzma')
 
     Returns:
         (bytes) Compressed data
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 08/10] binman: Add lzop bintool
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
                   ` (5 preceding siblings ...)
  2022-08-08 10:51 ` [PATCH v2 07/10] binman: Add gzip bintool Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 09/10] binman: Add xz bintool Stefan Herbrechtsmeier
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add lzop bintool to binman to support on-the-fly compression.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Added

 tools/binman/btool/lzop.py | 30 ++++++++++++++++++++++++++++++
 tools/binman/comp_util.py  | 18 ++++++++++--------
 2 files changed, 40 insertions(+), 8 deletions(-)
 create mode 100644 tools/binman/btool/lzop.py

diff --git a/tools/binman/btool/lzop.py b/tools/binman/btool/lzop.py
new file mode 100644
index 0000000000..f6903b4db7
--- /dev/null
+++ b/tools/binman/btool/lzop.py
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
+# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
+#
+"""Bintool implementation for lzop
+
+lzop allows compression and decompression of files.
+
+Documentation is available via::
+
+   man lzop
+"""
+
+from binman import bintool
+
+# pylint: disable=C0103
+class Bintoollzop(bintool.BintoolPacker):
+    """Compression/decompression using the lzop algorithm
+
+    This bintool supports running `lzop` to compress and decompress data, as
+    used by binman.
+
+    It is also possible to fetch the tool, which uses `apt` to install it.
+
+    Documentation is available via::
+
+        man lzop
+    """
+    def __init__(self, name):
+        super().__init__(name, 'lzo', compress_args=[])
diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
index 4fbd80e2ff..e7cba23aa8 100644
--- a/tools/binman/comp_util.py
+++ b/tools/binman/comp_util.py
@@ -11,12 +11,12 @@ from binman import bintool
 from patman import tools
 
 # Supported compressions
-COMPRESSIONS = ['bzip2', 'gzip', 'lz4', 'lzma']
+COMPRESSIONS = ['bzip2', 'gzip', 'lz4', 'lzma', 'lzo']
 
 bintools = {}
 
 def _get_tool_name(algo):
-    names = {'lzma': 'lzma_alone'}
+    names = {'lzma': 'lzma_alone', 'lzo': 'lzop'}
     return names.get(algo, algo)
 
 def _get_tool(algo):
@@ -34,13 +34,14 @@ def compress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'gzip', 'lz4' and 'lzma_alone' tools. It also
-    requires an output directory to be previously set up, by calling
+    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone' and 'lzop' tools. It
+    also requires an output directory to be previously set up, by calling
     PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to compress
-        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4' or 'lzma')
+        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma' or
+                    'lzo')
 
     Returns:
         bytes: Compressed data
@@ -61,13 +62,14 @@ def decompress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'gzip', 'lz4' and 'lzma_alone' tools. It also
-    requires an output directory to be previously set up, by calling
+    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone' and 'lzop' tools. It
+    also requires an output directory to be previously set up, by calling
     PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to decompress
-        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4' or 'lzma')
+        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma' or
+                    'lzo')
 
     Returns:
         (bytes) Compressed data
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 09/10] binman: Add xz bintool
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
                   ` (6 preceding siblings ...)
  2022-08-08 10:51 ` [PATCH v2 08/10] binman: Add lzop bintool Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-08 10:51 ` [PATCH v2 10/10] binman: Add zstd bintool Stefan Herbrechtsmeier
  2022-08-09 19:51 ` [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Simon Glass
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add xz bintool to binman to support on-the-fly compression.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Added

 tools/binman/btool/xz.py  | 31 +++++++++++++++++++++++++++++++
 tools/binman/comp_util.py | 18 +++++++++---------
 2 files changed, 40 insertions(+), 9 deletions(-)
 create mode 100644 tools/binman/btool/xz.py

diff --git a/tools/binman/btool/xz.py b/tools/binman/btool/xz.py
new file mode 100644
index 0000000000..e2b413d18b
--- /dev/null
+++ b/tools/binman/btool/xz.py
@@ -0,0 +1,31 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
+# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
+#
+"""Bintool implementation for xz
+
+xz allows compression and decompression of files.
+
+Documentation is available via::
+
+   man xz
+"""
+
+from binman import bintool
+
+# pylint: disable=C0103
+class Bintoolxz(bintool.BintoolPacker):
+    """Compression/decompression using the xz algorithm
+
+    This bintool supports running `xz` to compress and decompress data, as
+    used by binman.
+
+    It is also possible to fetch the tool, which uses `apt` to install it.
+
+    Documentation is available via::
+
+        man xz
+    """
+    def __init__(self, name):
+        super().__init__(name, fetch_package='xz-utils',
+                         version_regex=r'xz \(XZ Utils\) ([0-9.]+)')
diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
index e7cba23aa8..35835450e2 100644
--- a/tools/binman/comp_util.py
+++ b/tools/binman/comp_util.py
@@ -11,7 +11,7 @@ from binman import bintool
 from patman import tools
 
 # Supported compressions
-COMPRESSIONS = ['bzip2', 'gzip', 'lz4', 'lzma', 'lzo']
+COMPRESSIONS = ['bzip2', 'gzip', 'lz4', 'lzma', 'lzo', 'xz']
 
 bintools = {}
 
@@ -34,14 +34,14 @@ def compress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone' and 'lzop' tools. It
-    also requires an output directory to be previously set up, by calling
+    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone' 'lzop' and 'xz' tools.
+    It also requires an output directory to be previously set up, by calling
     PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to compress
-        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma' or
-                    'lzo')
+        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma',
+                    'lzo' or 'xz')
 
     Returns:
         bytes: Compressed data
@@ -62,14 +62,14 @@ def decompress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone' and 'lzop' tools. It
-    also requires an output directory to be previously set up, by calling
+    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop' and 'xz' tools.
+    It also requires an output directory to be previously set up, by calling
     PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to decompress
-        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma' or
-                    'lzo')
+        algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma',
+                    'lzo' or 'xz')
 
     Returns:
         (bytes) Compressed data
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH v2 10/10] binman: Add zstd bintool
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
                   ` (7 preceding siblings ...)
  2022-08-08 10:51 ` [PATCH v2 09/10] binman: Add xz bintool Stefan Herbrechtsmeier
@ 2022-08-08 10:51 ` Stefan Herbrechtsmeier
  2022-08-13 14:59   ` Simon Glass
  2022-08-09 19:51 ` [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Simon Glass
  9 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-08 10:51 UTC (permalink / raw)
  To: u-boot; +Cc: Stefan Herbrechtsmeier, Alper Nebi Yasak, Simon Glass

From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

Add zstd bintool to binman to support on-the-fly compression.

Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>

---

Changes in v2:
- Added

 tools/binman/btool/zstd.py     | 30 ++++++++++++++++++++++++++++++
 tools/binman/comp_util.py      | 18 +++++++++---------
 tools/binman/etype/blob_dtb.py |  4 ++++
 tools/binman/ftest.py          |  3 ++-
 4 files changed, 45 insertions(+), 10 deletions(-)
 create mode 100644 tools/binman/btool/zstd.py

diff --git a/tools/binman/btool/zstd.py b/tools/binman/btool/zstd.py
new file mode 100644
index 0000000000..299bd37126
--- /dev/null
+++ b/tools/binman/btool/zstd.py
@@ -0,0 +1,30 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
+# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
+#
+"""Bintool implementation for zstd
+
+zstd allows compression and decompression of files.
+
+Documentation is available via::
+
+   man zstd
+"""
+
+from binman import bintool
+
+# pylint: disable=C0103
+class Bintoolzstd(bintool.BintoolPacker):
+    """Compression/decompression using the zstd algorithm
+
+    This bintool supports running `zstd` to compress and decompress data, as
+    used by binman.
+
+    It is also possible to fetch the tool, which uses `apt` to install it.
+
+    Documentation is available via::
+
+        man zstd
+    """
+    def __init__(self, name):
+        super().__init__(name)
diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
index 35835450e2..0f15fae600 100644
--- a/tools/binman/comp_util.py
+++ b/tools/binman/comp_util.py
@@ -11,7 +11,7 @@ from binman import bintool
 from patman import tools
 
 # Supported compressions
-COMPRESSIONS = ['bzip2', 'gzip', 'lz4', 'lzma', 'lzo', 'xz']
+COMPRESSIONS = ['bzip2', 'gzip', 'lz4', 'lzma', 'lzo', 'xz', 'zstd']
 
 bintools = {}
 
@@ -34,14 +34,14 @@ def compress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone' 'lzop' and 'xz' tools.
-    It also requires an output directory to be previously set up, by calling
-    PrepareOutputDir().
+    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone' 'lzop', 'xz' and 'zstd'
+    tools. It also requires an output directory to be previously set up, by
+    calling PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to compress
         algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma',
-                    'lzo' or 'xz')
+                    'lzo', 'xz' or 'zstd')
 
     Returns:
         bytes: Compressed data
@@ -62,14 +62,14 @@ def decompress(indata, algo):
     Note that for lzma this uses an old version of the algorithm, not that
     provided by xz.
 
-    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop' and 'xz' tools.
-    It also requires an output directory to be previously set up, by calling
-    PrepareOutputDir().
+    This requires 'bzip2', 'gzip', 'lz4', 'lzma_alone', 'lzop', 'xz' and 'zstd'
+    tools. It also requires an output directory to be previously set up, by
+    calling PrepareOutputDir().
 
     Args:
         indata (bytes): Input data to decompress
         algo (str): Algorithm to use ('none', 'bzip2', 'gzip', 'lz4', 'lzma',
-                    'lzo' or 'xz')
+                    'lzo', 'xz' or 'zstd')
 
     Returns:
         (bytes) Compressed data
diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py
index 652b8abd8f..8d0b88d5b0 100644
--- a/tools/binman/etype/blob_dtb.py
+++ b/tools/binman/etype/blob_dtb.py
@@ -45,6 +45,10 @@ class Entry_blob_dtb(Entry_blob):
     def ProcessContents(self):
         """Re-read the DTB contents so that we get any calculated properties"""
         _, indata = state.GetFdtContents(self.GetFdtEtype())
+
+        if self.compress == 'zstd' and self.prepend != 'length':
+            self.Raise('The zstd compression requires a length header')
+
         data = self.CompressData(indata)
         if self.prepend == 'length':
             hdr = struct.pack('<I', len(data))
diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
index c9b67c48d6..98a1c7c9db 100644
--- a/tools/binman/ftest.py
+++ b/tools/binman/ftest.py
@@ -5266,7 +5266,8 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
 
     def testPadding(self):
         """Test padding of compression algorithms"""
-        for algo in comp_util.COMPRESSIONS:
+        # Skip zstd because it doesn't support padding
+        for algo in [a for a in comp_util.COMPRESSIONS if a != 'zstd']:
             data = comp_util.compress(COMPRESS_DATA, algo)
             data = data + bytes([0]) * 64
             orig = comp_util.decompress(data, algo)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available
  2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
                   ` (8 preceding siblings ...)
  2022-08-08 10:51 ` [PATCH v2 10/10] binman: Add zstd bintool Stefan Herbrechtsmeier
@ 2022-08-09 19:51 ` Simon Glass
  2022-08-10 21:32   ` Simon Glass
  9 siblings, 1 reply; 25+ messages in thread
From: Simon Glass @ 2022-08-09 19:51 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Skip tests which requires python elftools if the tool is not available.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Added
>
>  tools/binman/elf_test.py | 14 ++++++++++++++
>  tools/binman/ftest.py    | 18 ++++++++++++++++++
>  2 files changed, 32 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available
  2022-08-09 19:51 ` [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Simon Glass
@ 2022-08-10 21:32   ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-10 21:32 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Stefan,

On Tue, 9 Aug 2022 at 13:51, Simon Glass <sjg@chromium.org> wrote:
>
> On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
> >
> > From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> >
> > Skip tests which requires python elftools if the tool is not available.
> >
> > Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> >
> > ---
> >
> > Changes in v2:
> > - Added
> >
> >  tools/binman/elf_test.py | 14 ++++++++++++++
> >  tools/binman/ftest.py    | 18 ++++++++++++++++++
> >  2 files changed, 32 insertions(+)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

The rest of this series is going to need more investigation and
thought on my part. I will come back to you by the end of next week.

Regards,
SImon

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 02/10] binman: Make compressed data header optional
  2022-08-08 10:51 ` [PATCH v2 02/10] binman: Make compressed data header optional Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  2022-08-15  7:09     ` Stefan Herbrechtsmeier
  0 siblings, 1 reply; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Stefan,

On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Move the compressed data header handling into the dtb blob entry class
> and make it optional. The header is uncommon, not supported by U-Boot
> and incompatible with external compressed artifacts.
>
> If needed the header could be enabled with the following
> attribute beside the compress attribute:
>   prepend = "length";
>
> The header was introduced as part of commit eb0f4a4cb402 ("binman:
> Support replacing data in a cbfs") to allow device tree entries to be
> larger that the compressed contents. Regarding the commit "this is
> necessary to cope with a compressed device tree being updated in such a
> way that it shrinks after the entry size is already set (an obscure
> case)". This case need to be fixed without influence any compressed data
> by itself.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Reworked to make the feature optional.
>
>  tools/binman/cbfs_util.py                     |  8 ++---
>  tools/binman/comp_util.py                     | 11 ++----
>  tools/binman/entries.rst                      |  4 +++
>  tools/binman/entry.py                         |  2 +-
>  tools/binman/etype/blob_dtb.py                | 21 ++++++++++++
>  tools/binman/ftest.py                         | 34 ++++++++++++++++---
>  .../test/235_compress_prepend_length_dtb.dts  | 17 ++++++++++
>  7 files changed, 78 insertions(+), 19 deletions(-)
>  create mode 100644 tools/binman/test/235_compress_prepend_length_dtb.dts

I've been through this and I think it is a good change. We can use
your technique (property in the blob_dtb etype) to deal with any
future problems that come up.

But I would like to split this patch into three:

1. Add your blob_dtb property and the test
2. Change all uses of compress()/decompress() to call with add with_header=False
3. Drop the with_header arg from comp_util.py

A few more tweaks below.

>
> diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py
> index 9cad03886f..a1836f4ad3 100644
> --- a/tools/binman/cbfs_util.py
> +++ b/tools/binman/cbfs_util.py
> @@ -241,9 +241,9 @@ class CbfsFile(object):
>          """Handle decompressing data if necessary"""
>          indata = self.data
>          if self.compress == COMPRESS_LZ4:
> -            data = comp_util.decompress(indata, 'lz4', with_header=False)
> +            data = comp_util.decompress(indata, 'lz4')
>          elif self.compress == COMPRESS_LZMA:
> -            data = comp_util.decompress(indata, 'lzma', with_header=False)
> +            data = comp_util.decompress(indata, 'lzma')
>          else:
>              data = indata
>          self.memlen = len(data)
> @@ -362,9 +362,9 @@ class CbfsFile(object):
>          elif self.ftype == TYPE_RAW:
>              orig_data = data
>              if self.compress == COMPRESS_LZ4:
> -                data = comp_util.compress(orig_data, 'lz4', with_header=False)
> +                data = comp_util.compress(orig_data, 'lz4')
>              elif self.compress == COMPRESS_LZMA:
> -                data = comp_util.compress(orig_data, 'lzma', with_header=False)
> +                data = comp_util.compress(orig_data, 'lzma')
>              self.memlen = len(orig_data)
>              self.data_len = len(data)
>              attr = struct.pack(ATTR_COMPRESSION_FORMAT,
> diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
> index dc76adab35..269bbf7975 100644
> --- a/tools/binman/comp_util.py
> +++ b/tools/binman/comp_util.py
> @@ -3,7 +3,6 @@
>  #
>  """Utilities to compress and decompress data"""
>
> -import struct
>  import tempfile
>
>  from binman import bintool
> @@ -16,7 +15,7 @@ LZMA_ALONE = bintool.Bintool.create('lzma_alone')
>  HAVE_LZMA_ALONE = LZMA_ALONE.is_present()
>
>
> -def compress(indata, algo, with_header=True):
> +def compress(indata, algo):
>      """Compress some data using a given algorithm
>
>      Note that for lzma this uses an old version of the algorithm, not that
> @@ -41,12 +40,9 @@ def compress(indata, algo, with_header=True):
>          data = LZMA_ALONE.compress(indata)
>      else:
>          raise ValueError("Unknown algorithm '%s'" % algo)
> -    if with_header:
> -        hdr = struct.pack('<I', len(data))
> -        data = hdr + data
>      return data
>
> -def decompress(indata, algo, with_header=True):
> +def decompress(indata, algo):
>      """Decompress some data using a given algorithm
>
>      Note that for lzma this uses an old version of the algorithm, not that
> @@ -64,9 +60,6 @@ def decompress(indata, algo, with_header=True):
>      """
>      if algo == 'none':
>          return indata
> -    if with_header:
> -        data_len = struct.unpack('<I', indata[:4])[0]
> -        indata = indata[4:4 + data_len]
>      if algo == 'lz4':
>          data = LZ4.decompress(indata)
>      elif algo == 'lzma':
> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
> index ae4305c99e..8e722426d3 100644
> --- a/tools/binman/entries.rst
> +++ b/tools/binman/entries.rst
> @@ -208,6 +208,10 @@ This is a blob containing a device tree. The contents of the blob are
>  obtained from the list of available device-tree files, managed by the
>  'state' module.
>
> +Additional Properties / Entry arguments:
> +    - prepend: Header type to use:
> +        none: No header
> +        length: 32-bit length header
>
>
>  Entry: blob-ext: Externally built binary blob
> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
> index a07a588864..8cbfd43af9 100644
> --- a/tools/binman/entry.py
> +++ b/tools/binman/entry.py
> @@ -1069,7 +1069,7 @@ features to produce new behaviours.
>              indata: Data to compress
>
>          Returns:
> -            Compressed data (first word is the compressed size)
> +            Compressed data
>          """
>          self.uncomp_data = indata
>          if self.compress != 'none':
> diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py
> index 4159e3032a..652b8abd8f 100644
> --- a/tools/binman/etype/blob_dtb.py
> +++ b/tools/binman/etype/blob_dtb.py
> @@ -7,6 +7,8 @@
>
>  from binman.entry import Entry
>  from binman.etype.blob import Entry_blob
> +from dtoc import fdt_util
> +import struct
>
>  # This is imported if needed
>  state = None
> @@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob):
>      This is a blob containing a device tree. The contents of the blob are
>      obtained from the list of available device-tree files, managed by the
>      'state' module.
> +
> +    Additional attributes:
> +        prepend: Header used (e.g. 'length'), 'none' if none
>      """
>      def __init__(self, section, etype, node):
>          # Put this here to allow entry-docs and help to work without libfdt
> @@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob):
>
>          super().__init__(section, etype, node)
>
> +        self.prepend = 'none'

None ?

> +
> +    def ReadNode(self):
> +        super().ReadNode()
> +        self.prepend = fdt_util.GetString(self._node, 'prepend', 'none')

Can you drop the 'none' so that it uses None instead?

Aso we should check for a valid value here - e.g. it must be 'length'
and not something else, otherwise self.Raise()

> +
>      def ObtainContents(self):
>          """Get the device-tree from the list held by the 'state' module"""
>          self._filename = self.GetDefaultFilename()
> @@ -35,6 +46,9 @@ class Entry_blob_dtb(Entry_blob):
>          """Re-read the DTB contents so that we get any calculated properties"""
>          _, indata = state.GetFdtContents(self.GetFdtEtype())
>          data = self.CompressData(indata)
> +        if self.prepend == 'length':
> +            hdr = struct.pack('<I', len(data))
> +            data = hdr + data
>          return self.ProcessContentsUpdate(data)
>
>      def GetFdtEtype(self):
> @@ -50,6 +64,13 @@ class Entry_blob_dtb(Entry_blob):
>          fname = self.GetDefaultFilename()
>          return {self.GetFdtEtype(): [self, fname]}
>
> +    def ReadData(self, decomp=True, alt_format=None):
> +        data = super().ReadData(decomp, alt_format)
> +        if self.prepend == 'length':
> +            data_len = struct.unpack('<I', data[:4])[0]
> +            data = data[4:4 + data_len]
> +        return data
> +
>      def WriteData(self, data, decomp=True):
>          ok = super().WriteData(data, decomp)
>
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index da9aa9e679..057b4e28b7 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -2536,6 +2536,28 @@ class TestFunctional(unittest.TestCase):
>              }
>          self.assertEqual(expected, props)
>
> +    def testCompressPrependLengthDtb(self):
> +        """Test that compress of device-tree files with length header is
> +        supported
> +        """
> +        data = self.data = self._DoReadFileRealDtb('235_compress_prepend_length_dtb.dts')
> +        self.assertEqual(U_BOOT_DATA, data[:len(U_BOOT_DATA)])
> +        dtb_data = data[len(U_BOOT_DATA):]
> +        comp_data_len = struct.unpack('<I', dtb_data[:4])[0]
> +        comp_data = dtb_data[4:4 + comp_data_len]
> +        orig = self._decompress(comp_data)
> +        dtb = fdt.Fdt.FromData(orig)
> +        dtb.Scan()
> +        props = self._GetPropTree(dtb, ['size', 'uncomp-size'])
> +        expected = {
> +            'u-boot:size': len(U_BOOT_DATA),
> +            'u-boot-dtb:uncomp-size': len(orig),
> +            'u-boot-dtb:size': len(dtb_data),
> +            'size': len(data),
> +            }
> +        self.assertEqual(expected, props)
> +
> +
>      def testCbfsUpdateFdt(self):
>          """Test that we can update the device tree with CBFS offset/size info"""
>          self._CheckLz4()
> @@ -2856,7 +2878,7 @@ class TestFunctional(unittest.TestCase):
>      def testExtractCbfsRaw(self):
>          """Test extracting CBFS compressed data without decompressing it"""
>          data = self._RunExtractCmd('section/cbfs/u-boot-dtb', decomp=False)
> -        dtb = comp_util.decompress(data, 'lzma', with_header=False)
> +        dtb = comp_util.decompress(data, 'lzma')
>          self.assertEqual(EXTRACT_DTB_SIZE, len(dtb))
>
>      def testExtractBadEntry(self):
> @@ -4427,15 +4449,17 @@ 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)])
> +        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)])
> +        data2 = rest1[:len(expect2)]
> +        section2 = self._decompress(data2)
> +        self.assertEquals(expect2, data2)
>          self.assertEquals(COMPRESS_DATA + COMPRESS_DATA, section2)
>          rest2 = rest1[len(expect2):]
>
> diff --git a/tools/binman/test/235_compress_prepend_length_dtb.dts b/tools/binman/test/235_compress_prepend_length_dtb.dts
> new file mode 100644
> index 0000000000..a5abc60477
> --- /dev/null
> +++ b/tools/binman/test/235_compress_prepend_length_dtb.dts
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/dts-v1/;
> +
> +/ {
> +       #address-cells = <1>;
> +       #size-cells = <1>;
> +
> +       binman {
> +               u-boot {
> +               };
> +               u-boot-dtb {
> +                       compress = "lz4";
> +                       prepend = "length";
> +               };
> +       };
> +};
> --
> 2.30.2
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 03/10] binman: Simplify comp_util class
  2022-08-08 10:51 ` [PATCH v2 03/10] binman: Simplify comp_util class Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Stefan,

On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Simplify the comp_util class by remove duplicate code as preparation for
> additional compress algorithms.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
> ---
>
> (no changes since v1)
>
>  tools/binman/cbfs_util_test.py |  2 +-
>  tools/binman/comp_util.py      | 46 +++++++++++++++++++++++-----------
>  tools/binman/ftest.py          |  2 +-
>  3 files changed, 33 insertions(+), 17 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Please see below

>
> diff --git a/tools/binman/cbfs_util_test.py b/tools/binman/cbfs_util_test.py
> index f86b295149..44ebd04278 100755
> --- a/tools/binman/cbfs_util_test.py
> +++ b/tools/binman/cbfs_util_test.py
> @@ -50,7 +50,7 @@ class TestCbfs(unittest.TestCase):
>          cls.cbfstool = bintool.Bintool.create('cbfstool')
>          cls.have_cbfstool = cls.cbfstool.is_present()
>
> -        cls.have_lz4 = comp_util.HAVE_LZ4
> +        cls.have_lz4 = comp_util.is_present('lz4')
>
>      @classmethod
>      def tearDownClass(cls):
> diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
> index 269bbf7975..faa08a7f8a 100644
> --- a/tools/binman/comp_util.py
> +++ b/tools/binman/comp_util.py
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  # Copyright 2022 Google LLC
> +# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
> +# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>  #
>  """Utilities to compress and decompress data"""
>
> @@ -8,12 +10,23 @@ import tempfile
>  from binman import bintool
>  from patman import tools
>
> -LZ4 = bintool.Bintool.create('lz4')
> -HAVE_LZ4 = LZ4.is_present()
> +# Supported compressions
> +COMPRESSIONS = ['lz4', 'lzma']

How about ALGOS ?

'Compressions' is a bit of an odd word.

>
> -LZMA_ALONE = bintool.Bintool.create('lzma_alone')
> -HAVE_LZMA_ALONE = LZMA_ALONE.is_present()
> +bintools = {}
>
> +def _get_tool_name(algo):

function comment

> +    names = {'lzma': 'lzma_alone'}
> +    return names.get(algo, algo)
> +
> +def _get_tool(algo):

function comment

> +    global bintools
> +    name = _get_tool_name(algo)
> +    tool = bintools.get(name)
> +    if not tool:
> +        tool = bintool.Bintool.create(name)
> +        bintools[name] = tool
> +    return tool
>
>  def compress(indata, algo):
>      """Compress some data using a given algorithm
> @@ -33,13 +46,12 @@ def compress(indata, algo):
>      """
>      if algo == 'none':
>          return indata
> -    if algo == 'lz4':
> -        data = LZ4.compress(indata)
> -    # cbfstool uses a very old version of lzma
> -    elif algo == 'lzma':
> -        data = LZMA_ALONE.compress(indata)
> -    else:
> +    if algo not in COMPRESSIONS:
>          raise ValueError("Unknown algorithm '%s'" % algo)
> +
> +    tool = _get_tool(algo)
> +    data = tool.compress(indata)
> +
>      return data
>
>  def decompress(indata, algo):
> @@ -60,10 +72,14 @@ def decompress(indata, algo):
>      """
>      if algo == 'none':
>          return indata
> -    if algo == 'lz4':
> -        data = LZ4.decompress(indata)
> -    elif algo == 'lzma':
> -        data = LZMA_ALONE.decompress(indata)
> -    else:
> +    if algo not in COMPRESSIONS:
>          raise ValueError("Unknown algorithm '%s'" % algo)
> +
> +    tool = _get_tool(algo)
> +    data = tool.decompress(indata)
> +
>      return data
> +
> +def is_present(algo):
> +     tool = _get_tool(algo)
> +     return tool.is_present()
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 057b4e28b7..96c15cff77 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -212,7 +212,7 @@ class TestFunctional(unittest.TestCase):
>          TestFunctional._MakeInputFile('tee.elf',
>              tools.read_file(cls.ElfTestFile('elf_sections')))
>
> -        cls.have_lz4 = comp_util.HAVE_LZ4
> +        cls.have_lz4 = comp_util.is_present('lz4')
>
>      @classmethod
>      def tearDownClass(cls):
> --
> 2.30.2
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 04/10] binman: Add compression tests
  2022-08-08 10:51 ` [PATCH v2 04/10] binman: Add compression tests Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Stefan,

On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Add common test functions to test all supported compressions.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
> Instead of the for loop it is possible to use Parameterized [1] testing.
>
> [1] https://github.com/wolever/parameterized
>
> Changes in v2:
> - Added
>
>  tools/binman/ftest.py | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>

nits below

>
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 96c15cff77..c9b67c48d6 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -5248,6 +5248,30 @@ fdt         fdtmap                Extract the devicetree blob from the fdtmap
>              comp_util.decompress(b'1234', 'invalid')
>          self.assertIn("Unknown algorithm 'invalid'", str(e.exception))
>
> +    def testCompressions(self):
> +        """Test compression algorithms"""
> +        for algo in comp_util.COMPRESSIONS:
> +            data = comp_util.compress(COMPRESS_DATA, algo)
> +            self.assertNotEqual(COMPRESS_DATA, data)
> +            orig = comp_util.decompress(data, algo)
> +            self.assertEquals(COMPRESS_DATA, orig)
> +
> +    def testVersions(self):
> +        """Test tool version of compression algorithms"""
> +        for algo in comp_util.COMPRESSIONS:
> +            tool = comp_util._get_tool(algo)
> +            version = tool.version()
> +            print('%s - %s' % (algo, version))
> +            self.assertRegex(version, '^v?[0-9]+[0-9.]*')
> +
> +    def testPadding(self):
> +        """Test padding of compression algorithms"""
> +        for algo in comp_util.COMPRESSIONS:
> +            data = comp_util.compress(COMPRESS_DATA, algo)
> +            data = data + bytes([0]) * 64

tools.get_bytes(0, 64)

> +            orig = comp_util.decompress(data, algo)
> +            self.assertEquals(COMPRESS_DATA, orig)
> +
>      def testBintoolDocs(self):
>          """Test for creation of bintool documentation"""
>          with test_util.capture_sys_output() as (stdout, stderr):
> --
> 2.30.2
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 05/10] binman: Add BintoolPacker class to bintool
  2022-08-08 10:51 ` [PATCH v2 05/10] binman: Add BintoolPacker class to bintool Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  2022-08-15 14:43     ` Stefan Herbrechtsmeier
  0 siblings, 1 reply; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Stefan,

On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Add a bintools base class for packers which compression / decompression
> entry contents.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Added
>
>  tools/binman/bintool.py | 94 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 94 insertions(+)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

The one strange thing about this is that we don't support these
compression tools being missing, as we do with other tools. If 'lz4'
is needed and not present, binman just fails. This predates your
change, but I just noticed it.

I think this is OK though, at least for now. We could handle a missing
algo by returning empty data and marking the entry as 'missing', but I
don't see a great need for it at present. The compression tools are
widely available and easy to install.

nits below

> diff --git a/tools/binman/bintool.py b/tools/binman/bintool.py
> index 8435b29749..1d22962790 100644
> --- a/tools/binman/bintool.py
> +++ b/tools/binman/bintool.py
> @@ -1,5 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0+
>  # Copyright 2022 Google LLC
> +# Copyright (C) 2022 Weidmüller Interface GmbH & Co. KG
> +# Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>  #
>  """Base class for all bintools
>
> @@ -464,3 +466,95 @@ binaries. It is fairly easy to create new bintools. Just add a new file to the
>              str: Version string for this bintool
>          """
>          return 'unknown'
> +
> +class BintoolPacker(Bintool):
> +    """Tool which compression / decompression entry contents
> +
> +    This is a bintools base class for compression / decompression packer
> +    """
> +    def __init__(self, name, compression=None, compress_args=None,
> +                 decompress_args=None, fetch_package=None,
> +                 version_regex=r'(v[0-9.]+)'):

Can you document these?

> +        desc = '%s compression' % (compression if compression else name)
> +        super().__init__(name, desc)
> +        if compress_args is None:
> +            compress_args = ['--compress']
> +        self.compress_args = compress_args
> +        if decompress_args is None:
> +            decompress_args = ['--decompress']
> +        self.decompress_args = decompress_args
> +        if fetch_package is None:
> +            fetch_package = name
> +        self.fetch_package = fetch_package
> +        self.version_regex = version_regex
> +
> +    def compress(self, indata):
> +        """Compress data
> +
> +        Args:
> +            indata (bytes): Data to compress
> +
> +        Returns:
> +            bytes: Compressed data
> +        """
> +        with tempfile.NamedTemporaryFile(prefix='comp.tmp',
> +                                         dir=tools.get_output_dir()) as tmp:
> +            tools.write_file(tmp.name, indata)
> +            args = self.compress_args + ['--stdout', tmp.name]
> +            return self.run_cmd(*args, binary=True)
> +
> +    def decompress(self, indata):
> +        """Decompress data
> +
> +        Args:
> +            indata (bytes): Data to decompress
> +
> +        Returns:
> +            bytes: Decompressed data
> +        """
> +        with tempfile.NamedTemporaryFile(prefix='decomp.tmp',
> +                                         dir=tools.get_output_dir()) as inf:
> +            tools.write_file(inf.name, indata)
> +            args = self.decompress_args + ['--stdout', inf.name]
> +            return self.run_cmd(*args, binary=True)
> +
> +    def fetch(self, method):
> +        """Fetch handler
> +
> +        This installs the gzip package using the apt utility.
> +
> +        Args:
> +            method (FETCH_...): Method to use
> +
> +        Returns:
> +            True if the file was fetched and now installed, None if a method
> +            other than FETCH_BIN was requested
> +
> +        Raises:
> +            Valuerror: Fetching could not be completed
> +        """
> +        if method != FETCH_BIN:
> +            return None
> +        pkg = self.fetch_package if self.fetch_package else self.name
> +        return self.apt_install(pkg)
> +
> +    def version(self):
> +        """Version handler
> +
> +        Returns:
> +            str: Version number
> +        """
> +        import re
> +
> +        result = self.run_cmd_result('-V')
> +        if not result:
> +            return super().version()
> +
> +        out = result.stdout.strip()
> +        if not out:
> +            out = result.stderr.strip()
> +        if not out:
> +            return super().version()
> +
> +        m_version = re.search(self.version_regex, out)
> +        return m_version.group(1) if m_version else out
> --
> 2.30.2
>

Regards,
Simon

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 06/10] binman: Add bzip2 bintool
  2022-08-08 10:51 ` [PATCH v2 06/10] binman: Add bzip2 bintool Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Add bzip2 bintool to binman to support on-the-fly compression.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Added
>
>  tools/binman/btool/bzip2.py | 30 ++++++++++++++++++++++++++++++
>  tools/binman/comp_util.py   | 14 +++++++-------
>  2 files changed, 37 insertions(+), 7 deletions(-)
>  create mode 100644 tools/binman/btool/bzip2.py

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 07/10] binman: Add gzip bintool
  2022-08-08 10:51 ` [PATCH v2 07/10] binman: Add gzip bintool Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

On Mon, 8 Aug 2022 at 04:52, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Add gzip bintool to binman to support on-the-fly compression of Linux
> kernel images and FPGA bitstreams. The SPL basic fitImage implementation
> supports only gzip decompression.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Added
>
>  tools/binman/btool/gzip.py | 31 +++++++++++++++++++++++++++++++
>  tools/binman/comp_util.py  | 16 +++++++++-------
>  2 files changed, 40 insertions(+), 7 deletions(-)
>  create mode 100644 tools/binman/btool/gzip.py

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 08/10] binman: Add lzop bintool
  2022-08-08 10:51 ` [PATCH v2 08/10] binman: Add lzop bintool Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

On Mon, 8 Aug 2022 at 04:52, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Add lzop bintool to binman to support on-the-fly compression.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Added
>
>  tools/binman/btool/lzop.py | 30 ++++++++++++++++++++++++++++++
>  tools/binman/comp_util.py  | 18 ++++++++++--------
>  2 files changed, 40 insertions(+), 8 deletions(-)
>  create mode 100644 tools/binman/btool/lzop.py

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 09/10] binman: Add xz bintool
  2022-08-08 10:51 ` [PATCH v2 09/10] binman: Add xz bintool Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

On Mon, 8 Aug 2022 at 04:52, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Add xz bintool to binman to support on-the-fly compression.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Added
>
>  tools/binman/btool/xz.py  | 31 +++++++++++++++++++++++++++++++
>  tools/binman/comp_util.py | 18 +++++++++---------
>  2 files changed, 40 insertions(+), 9 deletions(-)
>  create mode 100644 tools/binman/btool/xz.py

Reviewed-by: Simon Glass <sjg@chromium.org>

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 10/10] binman: Add zstd bintool
  2022-08-08 10:51 ` [PATCH v2 10/10] binman: Add zstd bintool Stefan Herbrechtsmeier
@ 2022-08-13 14:59   ` Simon Glass
  2022-08-15 14:44     ` Stefan Herbrechtsmeier
  0 siblings, 1 reply; 25+ messages in thread
From: Simon Glass @ 2022-08-13 14:59 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Stefan,

On Mon, 8 Aug 2022 at 04:52, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> Add zstd bintool to binman to support on-the-fly compression.
>
> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>
> ---
>
> Changes in v2:
> - Added
>
>  tools/binman/btool/zstd.py     | 30 ++++++++++++++++++++++++++++++
>  tools/binman/comp_util.py      | 18 +++++++++---------
>  tools/binman/etype/blob_dtb.py |  4 ++++
>  tools/binman/ftest.py          |  3 ++-
>  4 files changed, 45 insertions(+), 10 deletions(-)
>  create mode 100644 tools/binman/btool/zstd.py

Reviewed-by: Simon Glass <sjg@chromium.org>

But as you noted this does cause CI issues:

ERROR: binman.ftest.TestFunctional.testCompressions (subunit.RemotedTestCase)
binman.ftest.TestFunctional.testCompressions
----------------------------------------------------------------------
testtools.testresult.real._StringException: Traceback (most recent call last):
TypeError: a bytes-like object is required, not 'NoneType'
======================================================================
FAIL: binman.ftest.TestFunctional.testVersions (subunit.RemotedTestCase)
binman.ftest.TestFunctional.testVersions
----------------------------------------------------------------------
testtools.testresult.real._StringException: stdout: {{{
bzip2 - 1.0.8
gzip - 1.10
lz4 - v1.9.2
lzma - 9.22 beta
lzo - v1.04
xz - 5.2.4
zstd - unknown
}}}
Traceback (most recent call last):
AssertionError: Regex didn't match: '^v?[0-9]+[0-9.]*' not found in 'unknown'

One option is to check if zstd is available, using is_present() in
your testCompressions() function.

Regards,
Simon

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 02/10] binman: Make compressed data header optional
  2022-08-13 14:59   ` Simon Glass
@ 2022-08-15  7:09     ` Stefan Herbrechtsmeier
  2022-08-15 17:37       ` Simon Glass
  0 siblings, 1 reply; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-15  7:09 UTC (permalink / raw)
  To: Simon Glass; +Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Simon,

Am 13.08.2022 um 16:59 schrieb Simon Glass:
> Hi Stefan,
> 
> On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>>
>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> Move the compressed data header handling into the dtb blob entry class
>> and make it optional. The header is uncommon, not supported by U-Boot
>> and incompatible with external compressed artifacts.
>>
>> If needed the header could be enabled with the following
>> attribute beside the compress attribute:
>>    prepend = "length";
>>
>> The header was introduced as part of commit eb0f4a4cb402 ("binman:
>> Support replacing data in a cbfs") to allow device tree entries to be
>> larger that the compressed contents. Regarding the commit "this is
>> necessary to cope with a compressed device tree being updated in such a
>> way that it shrinks after the entry size is already set (an obscure
>> case)". This case need to be fixed without influence any compressed data
>> by itself.
>>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> ---
>>
>> Changes in v2:
>> - Reworked to make the feature optional.
>>
>>   tools/binman/cbfs_util.py                     |  8 ++---
>>   tools/binman/comp_util.py                     | 11 ++----
>>   tools/binman/entries.rst                      |  4 +++
>>   tools/binman/entry.py                         |  2 +-
>>   tools/binman/etype/blob_dtb.py                | 21 ++++++++++++
>>   tools/binman/ftest.py                         | 34 ++++++++++++++++---
>>   .../test/235_compress_prepend_length_dtb.dts  | 17 ++++++++++
>>   7 files changed, 78 insertions(+), 19 deletions(-)
>>   create mode 100644 tools/binman/test/235_compress_prepend_length_dtb.dts
> 
> I've been through this and I think it is a good change. We can use
> your technique (property in the blob_dtb etype) to deal with any
> future problems that come up.
> 
> But I would like to split this patch into three:
> 
> 1. Add your blob_dtb property and the test
> 2. Change all uses of compress()/decompress() to call with add with_header=False
> 3. Drop the with_header arg from comp_util.py

Okay, I will split the commit.

> 
> A few more tweaks below.
> 
>>
>> diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py
>> index 9cad03886f..a1836f4ad3 100644
>> --- a/tools/binman/cbfs_util.py
>> +++ b/tools/binman/cbfs_util.py
>> @@ -241,9 +241,9 @@ class CbfsFile(object):
>>           """Handle decompressing data if necessary"""
>>           indata = self.data
>>           if self.compress == COMPRESS_LZ4:
>> -            data = comp_util.decompress(indata, 'lz4', with_header=False)
>> +            data = comp_util.decompress(indata, 'lz4')
>>           elif self.compress == COMPRESS_LZMA:
>> -            data = comp_util.decompress(indata, 'lzma', with_header=False)
>> +            data = comp_util.decompress(indata, 'lzma')
>>           else:
>>               data = indata
>>           self.memlen = len(data)
>> @@ -362,9 +362,9 @@ class CbfsFile(object):
>>           elif self.ftype == TYPE_RAW:
>>               orig_data = data
>>               if self.compress == COMPRESS_LZ4:
>> -                data = comp_util.compress(orig_data, 'lz4', with_header=False)
>> +                data = comp_util.compress(orig_data, 'lz4')
>>               elif self.compress == COMPRESS_LZMA:
>> -                data = comp_util.compress(orig_data, 'lzma', with_header=False)
>> +                data = comp_util.compress(orig_data, 'lzma')
>>               self.memlen = len(orig_data)
>>               self.data_len = len(data)
>>               attr = struct.pack(ATTR_COMPRESSION_FORMAT,
>> diff --git a/tools/binman/comp_util.py b/tools/binman/comp_util.py
>> index dc76adab35..269bbf7975 100644
>> --- a/tools/binman/comp_util.py
>> +++ b/tools/binman/comp_util.py
>> @@ -3,7 +3,6 @@
>>   #
>>   """Utilities to compress and decompress data"""
>>
>> -import struct
>>   import tempfile
>>
>>   from binman import bintool
>> @@ -16,7 +15,7 @@ LZMA_ALONE = bintool.Bintool.create('lzma_alone')
>>   HAVE_LZMA_ALONE = LZMA_ALONE.is_present()
>>
>>
>> -def compress(indata, algo, with_header=True):
>> +def compress(indata, algo):
>>       """Compress some data using a given algorithm
>>
>>       Note that for lzma this uses an old version of the algorithm, not that
>> @@ -41,12 +40,9 @@ def compress(indata, algo, with_header=True):
>>           data = LZMA_ALONE.compress(indata)
>>       else:
>>           raise ValueError("Unknown algorithm '%s'" % algo)
>> -    if with_header:
>> -        hdr = struct.pack('<I', len(data))
>> -        data = hdr + data
>>       return data
>>
>> -def decompress(indata, algo, with_header=True):
>> +def decompress(indata, algo):
>>       """Decompress some data using a given algorithm
>>
>>       Note that for lzma this uses an old version of the algorithm, not that
>> @@ -64,9 +60,6 @@ def decompress(indata, algo, with_header=True):
>>       """
>>       if algo == 'none':
>>           return indata
>> -    if with_header:
>> -        data_len = struct.unpack('<I', indata[:4])[0]
>> -        indata = indata[4:4 + data_len]
>>       if algo == 'lz4':
>>           data = LZ4.decompress(indata)
>>       elif algo == 'lzma':
>> diff --git a/tools/binman/entries.rst b/tools/binman/entries.rst
>> index ae4305c99e..8e722426d3 100644
>> --- a/tools/binman/entries.rst
>> +++ b/tools/binman/entries.rst
>> @@ -208,6 +208,10 @@ This is a blob containing a device tree. The contents of the blob are
>>   obtained from the list of available device-tree files, managed by the
>>   'state' module.
>>
>> +Additional Properties / Entry arguments:
>> +    - prepend: Header type to use:
>> +        none: No header
>> +        length: 32-bit length header
>>
>>
>>   Entry: blob-ext: Externally built binary blob
>> diff --git a/tools/binman/entry.py b/tools/binman/entry.py
>> index a07a588864..8cbfd43af9 100644
>> --- a/tools/binman/entry.py
>> +++ b/tools/binman/entry.py
>> @@ -1069,7 +1069,7 @@ features to produce new behaviours.
>>               indata: Data to compress
>>
>>           Returns:
>> -            Compressed data (first word is the compressed size)
>> +            Compressed data
>>           """
>>           self.uncomp_data = indata
>>           if self.compress != 'none':
>> diff --git a/tools/binman/etype/blob_dtb.py b/tools/binman/etype/blob_dtb.py
>> index 4159e3032a..652b8abd8f 100644
>> --- a/tools/binman/etype/blob_dtb.py
>> +++ b/tools/binman/etype/blob_dtb.py
>> @@ -7,6 +7,8 @@
>>
>>   from binman.entry import Entry
>>   from binman.etype.blob import Entry_blob
>> +from dtoc import fdt_util
>> +import struct
>>
>>   # This is imported if needed
>>   state = None
>> @@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob):
>>       This is a blob containing a device tree. The contents of the blob are
>>       obtained from the list of available device-tree files, managed by the
>>       'state' module.
>> +
>> +    Additional attributes:
>> +        prepend: Header used (e.g. 'length'), 'none' if none
>>       """
>>       def __init__(self, section, etype, node):
>>           # Put this here to allow entry-docs and help to work without libfdt
>> @@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob):
>>
>>           super().__init__(section, etype, node)
>>
>> +        self.prepend = 'none'
> 
> None ?

I copy this from the compress attribute. You only need one check to 
support a missing value or a 'none' value. But I don't need this check 
and can use None.

> 
>> +
>> +    def ReadNode(self):
>> +        super().ReadNode()
>> +        self.prepend = fdt_util.GetString(self._node, 'prepend', 'none')
> 
> Can you drop the 'none' so that it uses None instead?

Is 'none' a valid entry? Do we need to distinguish between 'none' and an 
invalid value?

> Aso we should check for a valid value here - e.g. it must be 'length'
> and not something else, otherwise self.Raise()

Okay. I will remove the 'none' and only support 'length'.

Regards
   Stefan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 05/10] binman: Add BintoolPacker class to bintool
  2022-08-13 14:59   ` Simon Glass
@ 2022-08-15 14:43     ` Stefan Herbrechtsmeier
  0 siblings, 0 replies; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-15 14:43 UTC (permalink / raw)
  To: Simon Glass; +Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Simon,

Am 13.08.2022 um 16:59 schrieb Simon Glass:
> Hi Stefan,
> 
> On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>>
>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> Add a bintools base class for packers which compression / decompression
>> entry contents.
>>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> ---
>>
>> Changes in v2:
>> - Added
>>
>>   tools/binman/bintool.py | 94 +++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 94 insertions(+)
>>
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> The one strange thing about this is that we don't support these
> compression tools being missing, as we do with other tools. If 'lz4'
> is needed and not present, binman just fails. This predates your
> change, but I just noticed it.
> 
> I think this is OK though, at least for now. We could handle a missing
> algo by returning empty data and marking the entry as 'missing', but I
> don't see a great need for it at present. The compression tools are
> widely available and easy to install.

I have add a commit to the series for it.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 10/10] binman: Add zstd bintool
  2022-08-13 14:59   ` Simon Glass
@ 2022-08-15 14:44     ` Stefan Herbrechtsmeier
  0 siblings, 0 replies; 25+ messages in thread
From: Stefan Herbrechtsmeier @ 2022-08-15 14:44 UTC (permalink / raw)
  To: Simon Glass; +Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Simon,

Am 13.08.2022 um 16:59 schrieb Simon Glass:
> Hi Stefan,
> 
> On Mon, 8 Aug 2022 at 04:52, Stefan Herbrechtsmeier
> <stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>>
>> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> Add zstd bintool to binman to support on-the-fly compression.
>>
>> Signed-off-by: Stefan Herbrechtsmeier <stefan.herbrechtsmeier@weidmueller.com>
>>
>> ---
>>
>> Changes in v2:
>> - Added
>>
>>   tools/binman/btool/zstd.py     | 30 ++++++++++++++++++++++++++++++
>>   tools/binman/comp_util.py      | 18 +++++++++---------
>>   tools/binman/etype/blob_dtb.py |  4 ++++
>>   tools/binman/ftest.py          |  3 ++-
>>   4 files changed, 45 insertions(+), 10 deletions(-)
>>   create mode 100644 tools/binman/btool/zstd.py
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 
> But as you noted this does cause CI issues:
> 
> ERROR: binman.ftest.TestFunctional.testCompressions (subunit.RemotedTestCase)
> binman.ftest.TestFunctional.testCompressions
> ----------------------------------------------------------------------
> testtools.testresult.real._StringException: Traceback (most recent call last):
> TypeError: a bytes-like object is required, not 'NoneType'
> ======================================================================
> FAIL: binman.ftest.TestFunctional.testVersions (subunit.RemotedTestCase)
> binman.ftest.TestFunctional.testVersions
> ----------------------------------------------------------------------
> testtools.testresult.real._StringException: stdout: {{{
> bzip2 - 1.0.8
> gzip - 1.10
> lz4 - v1.9.2
> lzma - 9.22 beta
> lzo - v1.04
> xz - 5.2.4
> zstd - unknown
> }}}
> Traceback (most recent call last):
> AssertionError: Regex didn't match: '^v?[0-9]+[0-9.]*' not found in 'unknown'
> 
> One option is to check if zstd is available, using is_present() in
> your testCompressions() function.

I have add a new function to check the present of the tools in each test.

Regards
   Stefan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH v2 02/10] binman: Make compressed data header optional
  2022-08-15  7:09     ` Stefan Herbrechtsmeier
@ 2022-08-15 17:37       ` Simon Glass
  0 siblings, 0 replies; 25+ messages in thread
From: Simon Glass @ 2022-08-15 17:37 UTC (permalink / raw)
  To: Stefan Herbrechtsmeier
  Cc: U-Boot Mailing List, Stefan Herbrechtsmeier, Alper Nebi Yasak

Hi Stefan,

On Mon, 15 Aug 2022 at 01:09, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss@weidmueller.com> wrote:
>
> Hi Simon,
>
> Am 13.08.2022 um 16:59 schrieb Simon Glass:
> > Hi Stefan,
> >
> > On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier

[..]

> >>   # This is imported if needed
> >>   state = None
> >> @@ -17,6 +19,9 @@ class Entry_blob_dtb(Entry_blob):
> >>       This is a blob containing a device tree. The contents of the blob are
> >>       obtained from the list of available device-tree files, managed by the
> >>       'state' module.
> >> +
> >> +    Additional attributes:
> >> +        prepend: Header used (e.g. 'length'), 'none' if none
> >>       """
> >>       def __init__(self, section, etype, node):
> >>           # Put this here to allow entry-docs and help to work without libfdt
> >> @@ -25,6 +30,12 @@ class Entry_blob_dtb(Entry_blob):
> >>
> >>           super().__init__(section, etype, node)
> >>
> >> +        self.prepend = 'none'
> >
> > None ?
>
> I copy this from the compress attribute. You only need one check to
> support a missing value or a 'none' value. But I don't need this check
> and can use None.

OK I see. The idea there was that people might want to explicitly say
'none'. I;m not sure how use that is, particularly with prepend, but
I'm OK with either way.

>
> >
> >> +
> >> +    def ReadNode(self):
> >> +        super().ReadNode()
> >> +        self.prepend = fdt_util.GetString(self._node, 'prepend', 'none')
> >
> > Can you drop the 'none' so that it uses None instead?
>
> Is 'none' a valid entry? Do we need to distinguish between 'none' and an
> invalid value?

Eventually we do...but for now bad things happen. See the TODO in
binman for some of that.
>
> > Aso we should check for a valid value here - e.g. it must be 'length'
> > and not something else, otherwise self.Raise()
>
> Okay. I will remove the 'none' and only support 'length'.

As above, up to you. I had forgotten about the compress thing.

Regards,
Simon

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2022-08-15 17:38 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-08 10:51 [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Stefan Herbrechtsmeier
2022-08-08 10:51 ` [PATCH v2 02/10] binman: Make compressed data header optional Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-15  7:09     ` Stefan Herbrechtsmeier
2022-08-15 17:37       ` Simon Glass
2022-08-08 10:51 ` [PATCH v2 03/10] binman: Simplify comp_util class Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-08 10:51 ` [PATCH v2 04/10] binman: Add compression tests Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-08 10:51 ` [PATCH v2 05/10] binman: Add BintoolPacker class to bintool Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-15 14:43     ` Stefan Herbrechtsmeier
2022-08-08 10:51 ` [PATCH v2 06/10] binman: Add bzip2 bintool Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-08 10:51 ` [PATCH v2 07/10] binman: Add gzip bintool Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-08 10:51 ` [PATCH v2 08/10] binman: Add lzop bintool Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-08 10:51 ` [PATCH v2 09/10] binman: Add xz bintool Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-08 10:51 ` [PATCH v2 10/10] binman: Add zstd bintool Stefan Herbrechtsmeier
2022-08-13 14:59   ` Simon Glass
2022-08-15 14:44     ` Stefan Herbrechtsmeier
2022-08-09 19:51 ` [PATCH v2 01/10] binman: Skip elf tests if python elftools is not available Simon Glass
2022-08-10 21:32   ` Simon Glass

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.