[PATCH v2 02/10] binman: Make compressed data header optional

Simon Glass sjg at chromium.org
Sat Aug 13 16:59:20 CEST 2022


Hi Stefan,

On Mon, 8 Aug 2022 at 04:51, Stefan Herbrechtsmeier
<stefan.herbrechtsmeier-oss at weidmueller.com> wrote:
>
> From: Stefan Herbrechtsmeier <stefan.herbrechtsmeier at 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 at 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


More information about the U-Boot mailing list