[PATCH 2/6] binman: Don't add compression attribute for uncompressed files

[Neha Malcom Francis]

Hi Simon

On 15/10/23 02:10, Simon Glass wrote:
> cbfsutil changed to skip adding a compression attribute if there is no
> compression. Adjust the binman implementation to do the same.
> 
> This mirrors commit 105cdf5625 in coreboot.
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
> 
>   tools/binman/cbfs_util.py      | 13 ++++++++-----
>   tools/binman/cbfs_util_test.py |  8 ++++----
>   tools/binman/ftest.py          |  6 +++---
>   3 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/binman/cbfs_util.py b/tools/binman/cbfs_util.py
> index fc56b40b7531..92d2add25146 100644
> --- a/tools/binman/cbfs_util.py
> +++ b/tools/binman/cbfs_util.py
> @@ -333,7 +333,8 @@ class CbfsFile(object):
>           if self.ftype == TYPE_STAGE:
>               pass
>           elif self.ftype == TYPE_RAW:
> -            hdr_len += ATTR_COMPRESSION_LEN
> +            if self.compress:
> +                hdr_len += ATTR_COMPRESSION_LEN
>           elif self.ftype == TYPE_EMPTY:
>               pass
>           else:
> @@ -369,9 +370,11 @@ class CbfsFile(object):
>                   data = self.comp_bintool.compress(orig_data)
>               self.memlen = len(orig_data)
>               self.data_len = len(data)
> -            attr = struct.pack(ATTR_COMPRESSION_FORMAT,
> -                               FILE_ATTR_TAG_COMPRESSION, ATTR_COMPRESSION_LEN,
> -                               self.compress, self.memlen)
> +            if self.compress:
> +                attr = struct.pack(ATTR_COMPRESSION_FORMAT,
> +                                   FILE_ATTR_TAG_COMPRESSION,
> +                                   ATTR_COMPRESSION_LEN, self.compress,
> +                                   self.memlen)
>           elif self.ftype == TYPE_EMPTY:
>               data = tools.get_bytes(self.erase_byte, self.size)
>           else:
> @@ -405,7 +408,7 @@ class CbfsFile(object):
>           if expected_len != actual_len:  # pragma: no cover
>               # Test coverage of this is not available since this should never
>               # happen. It probably indicates that get_header_len() is broken.
> -            raise ValueError("Internal error: CBFS file '%s': Expected headers of %#x bytes, got %#d" %
> +            raise ValueError("Internal error: CBFS file '%s': Expected headers of %#x bytes, got %#x" %
>                                (self.name, expected_len, actual_len))
>           return hdr + name + attr + pad + content + data, hdr_len
>   
> diff --git a/tools/binman/cbfs_util_test.py b/tools/binman/cbfs_util_test.py
> index ee951d10cf3b..c3babbc64e78 100755
> --- a/tools/binman/cbfs_util_test.py
> +++ b/tools/binman/cbfs_util_test.py
> @@ -96,7 +96,7 @@ class TestCbfs(unittest.TestCase):
>           self.assertEqual(arch, cbfs.arch)
>           return cbfs
>   
> -    def _check_uboot(self, cbfs, ftype=cbfs_util.TYPE_RAW, offset=0x38,
> +    def _check_uboot(self, cbfs, ftype=cbfs_util.TYPE_RAW, offset=0x28,
>                        data=U_BOOT_DATA, cbfs_offset=None):
>           """Check that the U-Boot file is as expected
>   
> @@ -122,7 +122,7 @@ class TestCbfs(unittest.TestCase):
>           self.assertEqual(len(data), cfile.memlen)
>           return cfile
>   
> -    def _check_dtb(self, cbfs, offset=0x38, data=U_BOOT_DTB_DATA,
> +    def _check_dtb(self, cbfs, offset=0x28, data=U_BOOT_DTB_DATA,
>                      cbfs_offset=None):
>           """Check that the U-Boot dtb file is as expected
>   
> @@ -598,8 +598,8 @@ class TestCbfs(unittest.TestCase):
>           data = cbw.get_data()
>   
>           cbfs = cbfs_util.CbfsReader(data)
> -        self.assertEqual(0x38, cbfs.files['u-boot'].cbfs_offset)
> -        self.assertEqual(0x78, cbfs.files['u-boot-dtb'].cbfs_offset)
> +        self.assertEqual(0x28, cbfs.files['u-boot'].cbfs_offset)
> +        self.assertEqual(0x68, cbfs.files['u-boot-dtb'].cbfs_offset)
>   
>   
>   if __name__ == '__main__':
> diff --git a/tools/binman/ftest.py b/tools/binman/ftest.py
> index 16156b741052..2875194d750e 100644
> --- a/tools/binman/ftest.py
> +++ b/tools/binman/ftest.py
> @@ -2670,9 +2670,9 @@ class TestFunctional(unittest.TestCase):
>               'cbfs/u-boot:offset': 0x38,
>               'cbfs/u-boot:uncomp-size': len(U_BOOT_DATA),
>               'cbfs/u-boot:image-pos': 0x38,
> -            'cbfs/u-boot-dtb:offset': 0xb8,
> +            'cbfs/u-boot-dtb:offset': 0xa8,
>               'cbfs/u-boot-dtb:size': len(U_BOOT_DATA),
> -            'cbfs/u-boot-dtb:image-pos': 0xb8,
> +            'cbfs/u-boot-dtb:image-pos': 0xa8,
>               }, props)
>   
>       def testCbfsBadType(self):
> @@ -2854,7 +2854,7 @@ class TestFunctional(unittest.TestCase):
>   '  u-boot                  0     4  u-boot             0',
>   '  section               100   %x  section          100' % section_size,
>   '    cbfs                100   400  cbfs               0',
> -'      u-boot            138     4  u-boot            38',
> +'      u-boot            128     4  u-boot            28',
>   '      u-boot-dtb        180   105  u-boot-dtb        80          3c9',
>   '    u-boot-dtb          500   %x  u-boot-dtb       400          3c9' % fdt_size,
>   '  fdtmap                %x   3bd  fdtmap           %x' %

Reviewed-by: Neha Malcom Francis

-- 
Thanking You
Neha Malcom Francis