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-...@weidmueller.com> wrote:

From: Stefan Herbrechtsmeier <stefan.herbrechtsme...@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.herbrechtsme...@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

Reply via email to