[PATCH v2 02/10] binman: Make compressed data header optional
Stefan Herbrechtsmeier
stefan.herbrechtsmeier-oss at weidmueller.com
Mon Aug 15 09:09:35 CEST 2022
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 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
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
More information about the U-Boot
mailing list