From cb73aed1f00ebafe2b9319ec20e41734da9d6b88 Mon Sep 17 00:00:00 2001 From: Tao Bao Date: Wed, 31 Jan 2018 17:32:40 -0800 Subject: [PATCH] releasetools: Refactor the condition checking for using imgdiff. In Transfer class, unbundle 'intact' with the monotonicity of the input ranges. Negate the logic of 'intact', and thus rename it to 'trimmed'. Move this property from an attribute of Transfer class as the one in RangeSet.extra. 'trimmed' indicates whether the source / target ranges have been modified after creating the Transfer() instance. The logic that determines whether we can apply imgdiff has been refactored and consolidated into BlockImageDiff.CanUseImgdiff(). Now both of the two paths call this single copy, i.e. the one that detects large APKs (before creating Transfer()'s), and the other that's about to generate the patch for a given Transfer instance. Bug: 68016761 Test: python -m unittest test_blockimgdiff Test: Generate an incremental BBOTA package. Change-Id: Id07195f63f1fa6c3af6e9091940d251cf09fa104 --- tools/releasetools/blockimgdiff.py | 76 ++++++++++++++++--------- tools/releasetools/test_blockimgdiff.py | 51 +++++++++++++++++ 2 files changed, 101 insertions(+), 26 deletions(-) diff --git a/tools/releasetools/blockimgdiff.py b/tools/releasetools/blockimgdiff.py index 931026b11..f1cb6dba8 100644 --- a/tools/releasetools/blockimgdiff.py +++ b/tools/releasetools/blockimgdiff.py @@ -191,7 +191,6 @@ class Transfer(object): self.tgt_sha1 = tgt_sha1 self.src_sha1 = src_sha1 self.style = style - self.intact = tgt_ranges.monotonic and src_ranges.monotonic # We use OrderedDict rather than dict so that the output is repeatable; # otherwise it would depend on the hash values of the Transfer objects. @@ -333,6 +332,45 @@ class BlockImageDiff(object): def max_stashed_size(self): return self._max_stashed_size + @staticmethod + def FileTypeSupportedByImgdiff(filename): + """Returns whether the file type is supported by imgdiff.""" + return filename.lower().endswith(('.apk', '.jar', '.zip')) + + def CanUseImgdiff(self, name, tgt_ranges, src_ranges): + """Checks whether we can apply imgdiff for the given RangeSets. + + For files in ZIP format (e.g., APKs, JARs, etc.) we would like to use + 'imgdiff -z' if possible. Because it usually produces significantly smaller + patches than bsdiff. + + This is permissible if all of the following conditions hold. + - The imgdiff hasn't been disabled by the caller (e.g. squashfs); + - The file type is supported by imgdiff; + - The source and target blocks are monotonic (i.e. the data is stored with + blocks in increasing order); + - We haven't removed any blocks from the source set. + + If all these conditions are satisfied, concatenating all the blocks in the + RangeSet in order will produce a valid ZIP file (plus possibly extra zeros + in the last block). imgdiff is fine with extra zeros at the end of the file. + + Args: + name: The filename to be diff'd. + tgt_ranges: The target RangeSet. + src_ranges: The source RangeSet. + + Returns: + A boolean result. + """ + return ( + not self.disable_imgdiff and + self.FileTypeSupportedByImgdiff(name) and + tgt_ranges.monotonic and + src_ranges.monotonic and + not tgt_ranges.extra.get('trimmed') and + not src_ranges.extra.get('trimmed')) + def Compute(self, prefix): # When looking for a source file to use as the diff input for a # target file, we try: @@ -711,28 +749,13 @@ class BlockImageDiff(object): # transfer is intact. assert not self.disable_imgdiff imgdiff = True - if not xf.intact: + if (xf.src_ranges.extra.get('trimmed') or + xf.tgt_ranges.extra.get('trimmed')): imgdiff = False xf.patch = None else: - # For files in zip format (eg, APKs, JARs, etc.) we would - # like to use imgdiff -z if possible (because it usually - # produces significantly smaller patches than bsdiff). - # This is permissible if: - # - # - imgdiff is not disabled, and - # - the source and target files are monotonic (ie, the - # data is stored with blocks in increasing order), and - # - we haven't removed any blocks from the source set. - # - # If these conditions are satisfied then appending all the - # blocks in the set together in order will produce a valid - # zip file (plus possibly extra zeros in the last block), - # which is what imgdiff needs to operate. (imgdiff is - # fine with extra zeros at the end of the file.) - imgdiff = (not self.disable_imgdiff and xf.intact and - xf.tgt_name.split(".")[-1].lower() - in ("apk", "jar", "zip")) + imgdiff = self.CanUseImgdiff( + xf.tgt_name, xf.tgt_ranges, xf.src_ranges) xf.style = "imgdiff" if imgdiff else "bsdiff" diff_queue.append((index, imgdiff, patch_num)) patch_num += 1 @@ -977,7 +1000,7 @@ class BlockImageDiff(object): out_of_order += 1 assert xf.src_ranges.overlaps(u.tgt_ranges) xf.src_ranges = xf.src_ranges.subtract(u.tgt_ranges) - xf.intact = False + xf.src_ranges.extra['trimmed'] = True if xf.style == "diff" and not xf.src_ranges: # nothing left to diff from; treat as new data @@ -1261,11 +1284,12 @@ class BlockImageDiff(object): style, by_id) return - if tgt_name.split(".")[-1].lower() in ("apk", "jar", "zip"): - split_enable = (not self.disable_imgdiff and src_ranges.monotonic and - tgt_ranges.monotonic) - if split_enable and (self.tgt.RangeSha1(tgt_ranges) != - self.src.RangeSha1(src_ranges)): + # Split large APKs with imgdiff, if possible. We're intentionally checking + # file types one more time (CanUseImgdiff() checks that as well), before + # calling the costly RangeSha1()s. + if (self.FileTypeSupportedByImgdiff(tgt_name) and + self.tgt.RangeSha1(tgt_ranges) != self.src.RangeSha1(src_ranges)): + if self.CanUseImgdiff(tgt_name, tgt_ranges, src_ranges): large_apks.append((tgt_name, src_name, tgt_ranges, src_ranges)) return diff --git a/tools/releasetools/test_blockimgdiff.py b/tools/releasetools/test_blockimgdiff.py index 7084e2153..4ed8356dd 100644 --- a/tools/releasetools/test_blockimgdiff.py +++ b/tools/releasetools/test_blockimgdiff.py @@ -172,3 +172,54 @@ class BlockImageDiffTest(unittest.TestCase): # Insufficient cache to stash 15 blocks (size * 0.8 < 15). common.OPTIONS.cache_size = 15 * 4096 self.assertEqual(15, block_image_diff.ReviseStashSize()) + + def test_FileTypeSupportedByImgdiff(self): + self.assertTrue( + BlockImageDiff.FileTypeSupportedByImgdiff( + "/system/priv-app/Settings/Settings.apk")) + self.assertTrue( + BlockImageDiff.FileTypeSupportedByImgdiff( + "/system/framework/am.jar")) + self.assertTrue( + BlockImageDiff.FileTypeSupportedByImgdiff( + "/system/etc/security/otacerts.zip")) + + self.assertFalse( + BlockImageDiff.FileTypeSupportedByImgdiff( + "/system/framework/arm/boot.oat")) + self.assertFalse( + BlockImageDiff.FileTypeSupportedByImgdiff( + "/system/priv-app/notanapk")) + + def test_CanUseImgdiff(self): + block_image_diff = BlockImageDiff(EmptyImage(), EmptyImage()) + self.assertTrue( + block_image_diff.CanUseImgdiff( + "/system/app/app1.apk", RangeSet("10-15"), RangeSet("0-5"))) + + def test_CanUseImgdiff_ineligible(self): + # Disabled by caller. + block_image_diff = BlockImageDiff(EmptyImage(), EmptyImage(), + disable_imgdiff=True) + self.assertFalse( + block_image_diff.CanUseImgdiff( + "/system/app/app1.apk", RangeSet("10-15"), RangeSet("0-5"))) + + # Unsupported file type. + block_image_diff = BlockImageDiff(EmptyImage(), EmptyImage()) + self.assertFalse( + block_image_diff.CanUseImgdiff( + "/system/bin/gzip", RangeSet("10-15"), RangeSet("0-5"))) + + # At least one of the ranges is in non-monotonic order. + self.assertFalse( + block_image_diff.CanUseImgdiff( + "/system/app/app2.apk", RangeSet("10-15"), + RangeSet("15-20 30 10-14"))) + + # At least one of the ranges has been modified. + src_ranges = RangeSet("0-5") + src_ranges.extra['trimmed'] = True + self.assertFalse( + block_image_diff.CanUseImgdiff( + "/vendor/app/app3.apk", RangeSet("10-15"), src_ranges))