paycheck: Improve minor_version checking.
1) We explicitly catch whether this field is not set. This means we
might fail payloads generated by an old delta_generator, but ensures
that we catch such a failure in current payload generation. Test
logic slightly restructured to reduce duplication.
2) Slight changes to the checker method signature, for better uniformity
with the rest of the code. This also lets us test that we actually
read the minor_version field.
BUG=chromium:508566
TEST=Unit tests (revised)
Change-Id: Ib2d1999964ba892ef778ffc16bd1ca1c7d02bcd5
Reviewed-on: https://chromium-review.googlesource.com/285446
Tested-by: Gilad Arnold <garnold@chromium.org>
Reviewed-by: Gilad Arnold <garnold@chromium.org>
Commit-Queue: Gilad Arnold <garnold@chromium.org>
diff --git a/scripts/update_payload/checker.py b/scripts/update_payload/checker.py
index f2aa7bb..5428f66 100644
--- a/scripts/update_payload/checker.py
+++ b/scripts/update_payload/checker.py
@@ -50,6 +50,12 @@
_DEFAULT_PUBKEY_FILE_NAME = os.path.join(os.path.dirname(__file__),
_DEFAULT_PUBKEY_BASE_NAME)
+# Supported minor version map to payload types allowed to be using them.
+_SUPPORTED_MINOR_VERSIONS = {
+ 0: (_TYPE_FULL,),
+ 1: (_TYPE_DELTA,),
+ 2: (_TYPE_DELTA,),
+}
#
# Helper functions.
@@ -311,6 +317,7 @@
self.old_kernel_fs_size = 0
self.new_rootfs_fs_size = 0
self.new_kernel_fs_size = 0
+ self.minor_version = None
@staticmethod
def _CheckElem(msg, name, report, is_mandatory, is_submsg, convert=str,
@@ -494,33 +501,27 @@
'%s (%d) <= (num %sblocks - 1 (%d)) * block_size (%d).' %
(length_name, length, block_name or '', num_blocks - 1, block_size))
- def _CheckMinorVersion(self, report, minor_version, payload_type):
- """Checks that the minor version matches the payload type.
+ def _CheckManifestMinorVersion(self, report):
+ """Checks the payload manifest minor_version field.
Args:
report: The report object to add to.
- minor_version: The minor version of the payload.
- payload_type: The type of payload (full or delta).
Raises:
- error.PayloadError if any of the checks fails.
+ error.PayloadError if any of the checks fail.
"""
- report.AddField('minor version', minor_version)
-
- if minor_version == 0:
- # Minor version 0 implies a full payload.
- if payload_type != _TYPE_FULL:
+ self.minor_version = self._CheckOptionalField(self.payload.manifest,
+ 'minor_version', report)
+ if self.minor_version in _SUPPORTED_MINOR_VERSIONS:
+ if self.payload_type not in _SUPPORTED_MINOR_VERSIONS[self.minor_version]:
raise error.PayloadError(
- 'Minor version 0 not compatible with payload type: %s.'
- % payload_type)
- elif minor_version in (1, 2):
- # Minor version 1 or 2 implies a delta payload.
- if payload_type != _TYPE_DELTA:
- raise error.PayloadError(
- 'Minor version %d not compatible with payload type: %s.'
- % (minor_version, payload_type))
+ 'Minor version %d not compatible with payload type %s.' %
+ (self.minor_version, self.payload_type))
+ elif self.minor_version is None:
+ raise error.PayloadError('Minor version is not set.')
else:
- raise error.PayloadError('Unsupported minor version: %d' % minor_version)
+ raise error.PayloadError('Unsupported minor version: %d' %
+ self.minor_version)
def _CheckManifest(self, report, rootfs_part_size=0, kernel_part_size=0):
"""Checks the payload manifest.
@@ -623,7 +624,7 @@
# Check: minor_version makes sense for the payload type. This check should
# run after the payload type has been set.
- self._CheckMinorVersion(report, manifest.minor_version, self.payload_type)
+ self._CheckManifestMinorVersion(report)
def _CheckLength(self, length, total_blocks, op_name, length_name):
"""Checks whether a length matches the space designated in extents.
@@ -943,23 +944,22 @@
(op_name, data_offset, prev_data_offset))
# Type-specific checks.
- minor_version = self.payload.manifest.minor_version
if op.type in (common.OpType.REPLACE, common.OpType.REPLACE_BZ):
self._CheckReplaceOperation(op, data_length, total_dst_blocks, op_name)
- elif op.type == common.OpType.MOVE and minor_version == 1:
+ elif op.type == common.OpType.MOVE and self.minor_version == 1:
self._CheckMoveOperation(op, data_offset, total_src_blocks,
total_dst_blocks, op_name)
- elif op.type == common.OpType.BSDIFF and minor_version == 1:
+ elif op.type == common.OpType.BSDIFF and self.minor_version == 1:
self._CheckBsdiffOperation(data_length, total_dst_blocks, op_name)
- elif op.type == common.OpType.SOURCE_COPY and minor_version == 2:
+ elif op.type == common.OpType.SOURCE_COPY and self.minor_version == 2:
self._CheckSourceCopyOperation(data_offset, total_src_blocks,
total_dst_blocks, op_name)
- elif op.type == common.OpType.SOURCE_BSDIFF and minor_version == 2:
+ elif op.type == common.OpType.SOURCE_BSDIFF and self.minor_version == 2:
self._CheckBsdiffOperation(data_length, total_dst_blocks, op_name)
else:
raise error.PayloadError(
'Operation %s (type %d) not allowed in minor version %d' %
- (op_name, op.type, minor_version))
+ (op_name, op.type, self.minor_version))
return data_length if data_length is not None else 0
def _SizeToNumBlocks(self, size):
diff --git a/scripts/update_payload/checker_unittest.py b/scripts/update_payload/checker_unittest.py
index b57c2fc..871f252 100755
--- a/scripts/update_payload/checker_unittest.py
+++ b/scripts/update_payload/checker_unittest.py
@@ -458,6 +458,9 @@
False, True, new_rootfs_fs_size,
None if fail_bad_nri else hashlib.sha256('fake-nri-content').digest())
+ # Set the minor version.
+ payload_gen.SetMinorVersion(0)
+
# Create the test object.
payload_checker = _GetPayloadChecker(payload_gen.WriteToFile)
report = checker._PayloadReport()
@@ -860,11 +863,11 @@
total_src_blocks = 16
if op_type in (common.OpType.REPLACE, common.OpType.REPLACE_BZ):
- payload.manifest.minor_version = 0
+ payload_checker.minor_version = 0
elif op_type in (common.OpType.MOVE, common.OpType.BSDIFF):
- payload.manifest.minor_version = 2 if fail_bad_minor_version else 1
+ payload_checker.minor_version = 2 if fail_bad_minor_version else 1
elif op_type in (common.OpType.SOURCE_COPY, common.OpType.SOURCE_BSDIFF):
- payload.manifest.minor_version = 1 if fail_bad_minor_version else 2
+ payload_checker.minor_version = 1 if fail_bad_minor_version else 2
if op_type not in (common.OpType.MOVE, common.OpType.SOURCE_COPY):
if not fail_mismatched_data_offset_length:
@@ -999,6 +1002,7 @@
hashlib.sha256('fake-new-rootfs-content').digest())
payload_gen.SetPartInfo(True, True, kernel_part_size,
hashlib.sha256('fake-new-kernel-content').digest())
+ payload_gen.SetMinorVersion(0)
payload_gen.AddOperationWithData(
False, common.OpType.REPLACE,
dst_extents=[(0, rootfs_part_size / block_size)],
@@ -1055,28 +1059,31 @@
else:
self.assertIsNone(payload_checker._CheckSignatures(*args))
- def DoCheckMinorVersionTest(self, minor_version, payload_type):
- """Parametric testing for CheckMinorVersion().
+ def DoCheckManifestMinorVersionTest(self, minor_version, payload_type):
+ """Parametric testing for CheckManifestMinorVersion().
Args:
minor_version: The payload minor version to test with.
payload_type: The type of the payload we're testing, delta or full.
"""
# Create the test object.
- payload_checker = checker.PayloadChecker(self.MockPayload())
+ payload = self.MockPayload()
+ payload.manifest.minor_version = minor_version
+ payload_checker = checker.PayloadChecker(payload)
+ payload_checker.payload_type = payload_type
report = checker._PayloadReport()
should_succeed = (
(minor_version == 0 and payload_type == checker._TYPE_FULL) or
(minor_version == 1 and payload_type == checker._TYPE_DELTA) or
(minor_version == 2 and payload_type == checker._TYPE_DELTA))
- args = (report, minor_version, payload_type)
+ args = (report,)
if should_succeed:
- self.assertIsNone(payload_checker._CheckMinorVersion(*args))
+ self.assertIsNone(payload_checker._CheckManifestMinorVersion(*args))
else:
self.assertRaises(update_payload.PayloadError,
- payload_checker._CheckMinorVersion, *args)
+ payload_checker._CheckManifestMinorVersion, *args)
def DoRunTest(self, fail_wrong_payload_type, fail_invalid_block_size,
fail_mismatched_block_size, fail_excess_data):
@@ -1095,6 +1102,7 @@
hashlib.sha256('fake-new-rootfs-content').digest())
payload_gen.SetPartInfo(True, True, kernel_part_size,
hashlib.sha256('fake-new-kernel-content').digest())
+ payload_gen.SetMinorVersion(0)
payload_gen.AddOperationWithData(
False, common.OpType.REPLACE,
dst_extents=[(0, rootfs_part_size / block_size)],
@@ -1267,9 +1275,9 @@
'fail_unknown_sig_version': (True, False),
'fail_incorrect_sig': (True, False)})
- # Add all _CheckMinorVersion() test cases.
- AddParametricTests('CheckMinorVersion',
- {'minor_version': (0, 1, 2, 555),
+ # Add all _CheckManifestMinorVersion() test cases.
+ AddParametricTests('CheckManifestMinorVersion',
+ {'minor_version': (None, 0, 1, 2, 555),
'payload_type': (checker._TYPE_FULL,
checker._TYPE_DELTA)})
diff --git a/scripts/update_payload/test_utils.py b/scripts/update_payload/test_utils.py
index bdd6b3f..4e7881d 100644
--- a/scripts/update_payload/test_utils.py
+++ b/scripts/update_payload/test_utils.py
@@ -208,6 +208,10 @@
_SetMsgField(self.manifest, 'signatures_offset', sigs_offset)
_SetMsgField(self.manifest, 'signatures_size', sigs_size)
+ def SetMinorVersion(self, minor_version):
+ """Set the payload's minor version field."""
+ _SetMsgField(self.manifest, 'minor_version', minor_version)
+
def _WriteHeaderToFile(self, file_obj, manifest_len):
"""Writes a payload heaer to a file."""
# We need to access protected members in Payload for writing the header.