diff --git a/CHANGELOG.md b/CHANGELOG.md index ab83f3b..8ad55ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,11 +13,15 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## Fixed - The large file flag was not set properly in the source handler for large file transfers. +- The CRC algorithms will now be used for empty files as well instead of hardcoding the + checksum type to the NULL checksum. This was a bug which did not show directly for + checksums like CRC32 because those have an initial value of 0x0 ## Changed - Added `file_size` abstract method to `VirtualFilestore` - Renamed `HostFilestore` to `NativeFilestore`, but keep old name alias for backwards compatibility. +- Added `calculate_checksum` and `verify_checksum` to `VirtualFilestore` interface. # [v0.1.2] 2024-06-04 diff --git a/docs/api/cfdp.rst b/docs/api/cfdp.rst index d76bb3d..8f2131a 100644 --- a/docs/api/cfdp.rst +++ b/docs/api/cfdp.rst @@ -50,6 +50,14 @@ Exceptions Module :undoc-members: :show-inheritance: +CRC Module +------------------------------------------- + +.. automodule:: cfdppy.crc + :members: + :undoc-members: + :show-inheritance: + Definitions Module -------------------------- diff --git a/src/cfdppy/crc.py b/src/cfdppy/crc.py new file mode 100644 index 0000000..9c5d8d9 --- /dev/null +++ b/src/cfdppy/crc.py @@ -0,0 +1,19 @@ +import struct +from pathlib import Path + + +def calc_modular_checksum(file_path: Path) -> bytes: + """Calculates the modular checksum for a file in one go.""" + checksum = 0 + + with open(file_path, "rb") as file: + while True: + data = file.read(4) + if not data: + break + checksum += int.from_bytes( + data.ljust(4, b"\0"), byteorder="big", signed=False + ) + + checksum %= 2**32 + return struct.pack("!I", checksum) diff --git a/src/cfdppy/filestore.py b/src/cfdppy/filestore.py index ab47f2d..55d26a3 100644 --- a/src/cfdppy/filestore.py +++ b/src/cfdppy/filestore.py @@ -6,7 +6,12 @@ from pathlib import Path from typing import Optional, BinaryIO +from cfdppy.crc import calc_modular_checksum +from crcmod.predefined import PredefinedCrc +from spacepackets.cfdp.defs import NULL_CHECKSUM_U32, ChecksumType from spacepackets.cfdp.tlv import FilestoreResponseStatusCode +from cfdppy.exceptions import ChecksumNotImplemented, SourceFileDoesNotExist + _LOGGER = logging.getLogger(__name__) @@ -101,6 +106,29 @@ def list_directory( _LOGGER.warning("Listing directory not implemented in virtual filestore") return FilestoreResponseStatusCode.NOT_PERFORMED + @abc.abstractmethod + def calc_checksum( + self, + checksum_type: ChecksumType, + file_path: Path, + file_sz: int, + segment_len: int = 4096, + ) -> bytes: + pass + + def verify_checksum( + self, + checksum: bytes, + checksum_type: ChecksumType, + file_path: Path, + file_sz: int, + segment_len: int = 4096, + ) -> bool: + return ( + self.calc_checksum(checksum_type, file_path, file_sz, segment_len) + == checksum + ) + class NativeFilestore(VirtualFilestore): def __init__(self): @@ -265,5 +293,51 @@ def list_directory( os.chdir(curr_path) return FilestoreResponseStatusCode.SUCCESS + def _verify_checksum(self, checksum_type: ChecksumType): + if checksum_type not in [ + ChecksumType.CRC_32, + ChecksumType.CRC_32C, + ]: + raise ChecksumNotImplemented(checksum_type) + + def checksum_type_to_crcmod_str(self, checksum_type: ChecksumType) -> Optional[str]: + if checksum_type == ChecksumType.CRC_32: + return "crc32" + elif checksum_type == ChecksumType.CRC_32C: + return "crc32c" + raise ChecksumNotImplemented(checksum_type) + + def _generate_crc_calculator(self, checksum_type: ChecksumType) -> PredefinedCrc: + self._verify_checksum(checksum_type) + return PredefinedCrc(self.checksum_type_to_crcmod_str(checksum_type)) + + def calc_checksum( + self, + checksum_type: ChecksumType, + file_path: Path, + file_sz: int, + segment_len: int = 4096, + ) -> bytes: + if checksum_type == ChecksumType.NULL_CHECKSUM: + return NULL_CHECKSUM_U32 + elif checksum_type == ChecksumType.MODULAR: + return calc_modular_checksum(file_path) + crc_obj = self._generate_crc_calculator(checksum_type) + if segment_len == 0: + raise ValueError("Segment length can not be 0") + if not file_path.exists(): + raise SourceFileDoesNotExist(file_path) + current_offset = 0 + # Calculate the file CRC + with open(file_path, "rb") as file: + while current_offset < file_sz: + read_len = min(segment_len, file_sz - current_offset) + if read_len > 0: + crc_obj.update( + self.read_from_opened_file(file, current_offset, read_len) + ) + current_offset += read_len + return crc_obj.digest() + HostFilestore = NativeFilestore diff --git a/src/cfdppy/handler/crc.py b/src/cfdppy/handler/crc.py deleted file mode 100644 index 396c452..0000000 --- a/src/cfdppy/handler/crc.py +++ /dev/null @@ -1,79 +0,0 @@ -import struct -from pathlib import Path -from typing import Optional - -from crcmod.predefined import PredefinedCrc - -from spacepackets.cfdp import ChecksumType, NULL_CHECKSUM_U32 -from cfdppy.filestore import VirtualFilestore -from cfdppy.exceptions import ChecksumNotImplemented, SourceFileDoesNotExist - - -def calc_modular_checksum(file_path: Path) -> bytes: - """Calculates the modular checksum for a file in one go.""" - checksum = 0 - - with open(file_path, "rb") as file: - while True: - data = file.read(4) - if not data: - break - checksum += int.from_bytes( - data.ljust(4, b"\0"), byteorder="big", signed=False - ) - - checksum %= 2**32 - return struct.pack("!I", checksum) - - -class CrcHelper: - def __init__(self, init_type: ChecksumType, vfs: VirtualFilestore): - self.checksum_type = init_type - self.vfs = vfs - - def _verify_checksum(self): - if self.checksum_type not in [ - ChecksumType.NULL_CHECKSUM, - ChecksumType.CRC_32, - ChecksumType.CRC_32C, - ]: - raise ChecksumNotImplemented(self.checksum_type) - - def checksum_type_to_crcmod_str(self) -> Optional[str]: - if self.checksum_type == ChecksumType.NULL_CHECKSUM: - return None - if self.checksum_type == ChecksumType.CRC_32: - return "crc32" - elif self.checksum_type == ChecksumType.CRC_32C: - return "crc32c" - - def generate_crc_calculator(self) -> PredefinedCrc: - self._verify_checksum() - return PredefinedCrc(self.checksum_type_to_crcmod_str()) - - def calc_for_file( - self, file_path: Path, file_sz: int, segment_len: int = 4096 - ) -> bytes: - if self.checksum_type == ChecksumType.NULL_CHECKSUM: - return NULL_CHECKSUM_U32 - elif self.checksum_type == ChecksumType.MODULAR: - return calc_modular_checksum(file_path) - crc_obj = self.generate_crc_calculator() - if segment_len == 0: - raise ValueError("Segment length can not be 0") - if not file_path.exists(): - raise SourceFileDoesNotExist(file_path) - current_offset = 0 - # Calculate the file CRC - with open(file_path, "rb") as file: - while current_offset < file_sz: - if current_offset + segment_len > file_sz: - read_len = file_sz - current_offset - else: - read_len = segment_len - if read_len > 0: - crc_obj.update( - self.vfs.read_from_opened_file(file, current_offset, read_len) - ) - current_offset += read_len - return crc_obj.digest() diff --git a/src/cfdppy/handler/dest.py b/src/cfdppy/handler/dest.py index 1d0a517..010a788 100644 --- a/src/cfdppy/handler/dest.py +++ b/src/cfdppy/handler/dest.py @@ -23,7 +23,6 @@ _PositiveAckProcedureParams, get_packet_destination, ) -from cfdppy.handler.crc import CrcHelper from cfdppy.handler.defs import ( _FileParamsBase, ) @@ -240,6 +239,7 @@ def __init__(self): self.check_timer: Optional[Countdown] = None self.current_check_count: int = 0 self.closure_requested: bool = False + self.checksum_type: ChecksumType = ChecksumType.NULL_CHECKSUM self.finished_params: FinishedParams = FinishedParams( delivery_code=DeliveryCode.DATA_INCOMPLETE, file_status=FileStatus.FILE_STATUS_UNREPORTED, @@ -255,23 +255,6 @@ def __init__(self): self.positive_ack_params = _PositiveAckProcedureParams() self.last_inserted_packet = PduHolder(None) - def reset(self): - self.transaction_id = None - self.closure_requested = False - self.pdu_conf = PduConfig.empty() - self.finished_params = FinishedParams( - condition_code=ConditionCode.NO_ERROR, - delivery_code=DeliveryCode.DATA_INCOMPLETE, - file_status=FileStatus.FILE_STATUS_UNREPORTED, - ) - self.finished_params.file_status = FileStatus.FILE_STATUS_UNREPORTED - self.completion_disposition = CompletionDisposition.COMPLETED - self.fp.reset() - self.acked_params = _AckedModeParams() - self.remote_cfg = None - self.last_inserted_packet.pdu = None - self.current_check_count = 0 - class FsmResult: def __init__(self, states: DestStateWrapper): @@ -339,9 +322,9 @@ def __init__( self.user = user self.check_timer_provider = check_timer_provider self._params = _DestFieldWrapper() - self._cksum_verif_helper: CrcHelper = CrcHelper( - ChecksumType.NULL_CHECKSUM, user.vfs - ) + # self._cksum_verif_helper: CrcHelper = CrcHelper( + # ChecksumType.NULL_CHECKSUM, user.vfs + # ) self._pdus_to_be_sent: Deque[PduHolder] = deque() @property @@ -499,7 +482,7 @@ def reset(self): """This function is public to allow completely resetting the handler, but it is explicitely discouraged to do this. CFDP generally has mechanism to detect issues and errors on itself. """ - self._params.reset() + self._params = _DestFieldWrapper() self._pdus_to_be_sent.clear() self.states.state = CfdpState.IDLE self.states.step = TransactionStep.IDLE @@ -596,7 +579,7 @@ def _fsm_advancement_after_packets_were_sent(self): def _start_transaction(self, metadata_pdu: MetadataPdu) -> bool: if self.states.state != CfdpState.IDLE: return False - self._params.reset() + self._params = _DestFieldWrapper() self._common_first_packet_handler(metadata_pdu) self._handle_metadata_packet(metadata_pdu) return True @@ -686,7 +669,7 @@ def _handle_fd_without_previous_metadata( ) def _common_first_packet_not_metadata_pdu_handler(self, pdu: GenericPduPacket): - self._params.reset() + self._params = _DestFieldWrapper() self._common_first_packet_handler(pdu) self.states.step = TransactionStep.WAITING_FOR_METADATA self._params.acked_params.metadata_missing = True @@ -705,7 +688,7 @@ def _common_first_packet_handler(self, pdu: GenericPduPacket): self._params.remote_cfg = self.remote_cfg_table.get_cfg(pdu.source_entity_id) def _handle_metadata_packet(self, metadata_pdu: MetadataPdu): - self._cksum_verif_helper.checksum_type = metadata_pdu.checksum_type + self._params.checksum_type = metadata_pdu.checksum_type self._params.closure_requested = metadata_pdu.closure_requested self._params.acked_params.metadata_missing = False if metadata_pdu.dest_file_name is None or metadata_pdu.source_file_name is None: @@ -1090,13 +1073,15 @@ def _prepare_eof_ack_packet(self): def _checksum_verify(self) -> bool: file_delivery_complete = False if ( - self._cksum_verif_helper.checksum_type == ChecksumType.NULL_CHECKSUM + self._params.checksum_type == ChecksumType.NULL_CHECKSUM or self._params.fp.metadata_only ): file_delivery_complete = True else: - crc32 = self._cksum_verif_helper.calc_for_file( - self._params.fp.file_name, self._params.fp.progress + crc32 = self.user.vfs.calc_checksum( + self._params.checksum_type, + self._params.fp.file_name, + self._params.fp.progress, ) if crc32 == self._params.fp.crc32: file_delivery_complete = True diff --git a/src/cfdppy/handler/source.py b/src/cfdppy/handler/source.py index 6ba81ed..b869cab 100644 --- a/src/cfdppy/handler/source.py +++ b/src/cfdppy/handler/source.py @@ -28,7 +28,6 @@ UnretrievedPdusToBeSent, ) from cfdppy.handler.common import _PositiveAckProcedureParams -from cfdppy.handler.crc import CrcHelper from cfdppy.handler.defs import ( _FileParamsBase, ) @@ -36,8 +35,6 @@ from cfdppy.request import PutRequest from cfdppy.user import TransactionFinishedParams, TransactionParams from spacepackets.cfdp import ( - NULL_CHECKSUM_U32, - ChecksumType, ConditionCode, CrcFlag, Direction, @@ -49,6 +46,7 @@ TransactionId, TransmissionMode, ) +from spacepackets.cfdp.defs import ChecksumType from spacepackets.cfdp.pdu import ( AbstractFileDirectiveBase, AckPdu, @@ -251,7 +249,7 @@ def __init__( self.seq_num_provider = seq_num_provider self.check_timer_provider = check_timer_provider self._params = _TransferFieldWrapper(cfg.local_entity_id) - self._crc_helper = CrcHelper(ChecksumType.NULL_CHECKSUM, self.user.vfs) + # self._crc_helper = CrcHelper(ChecksumType.NULL_CHECKSUM, self.user.vfs) self._put_req: Optional[PutRequest] = None self._inserted_pdu = PduHolder(None) self._pdus_to_be_sent: Deque[PduHolder] = deque() @@ -636,9 +634,10 @@ def _prepare_metadata_pdu(self): assert self._put_req is not None options = [] if self._put_req.metadata_only: + assert self._params.remote_cfg is not None params = MetadataParams( closure_requested=self._params.closure_requested, - checksum_type=self._crc_helper.checksum_type, + checksum_type=ChecksumType.NULL_CHECKSUM, file_size=0, dest_file_name=None, source_file_name=None, @@ -662,10 +661,11 @@ def _prepare_metadata_pdu(self): ) def _prepare_metadata_base_params_with_metadata(self) -> MetadataParams: + assert self._params.remote_cfg is not None return MetadataParams( dest_file_name=self._put_req.dest_file.as_posix(), # type: ignore source_file_name=self._put_req.source_file.as_posix(), # type: ignore - checksum_type=self._crc_helper.checksum_type, + checksum_type=self._params.remote_cfg.crc_type, closure_requested=self._params.closure_requested, file_size=self._params.fp.file_size, ) @@ -890,7 +890,6 @@ def _setup_transmission_params(self): # This also sets the field of the PDU configuration struct. self._params.transmission_mode = trans_mode_to_set self._params.closure_requested = closure_req_to_set - self._crc_helper.checksum_type = self._params.remote_cfg.crc_type def _add_packet_to_be_sent(self, packet: GenericPduPacket): self._pdus_to_be_sent.append(PduHolder(packet)) @@ -1005,15 +1004,13 @@ def _abandon_transaction(self): self.reset() def _checksum_calculation(self, size_to_calculate: int) -> bytes: - if self._params.fp.file_size == 0: - # Empty file, use null checksum - crc = NULL_CHECKSUM_U32 - else: - assert self._put_req is not None - assert self._put_req.source_file is not None - crc = self._crc_helper.calc_for_file( - file_path=self._put_req.source_file, - file_sz=size_to_calculate, - segment_len=self._params.fp.segment_len, - ) - return crc + assert self._put_req is not None + assert self._put_req.source_file is not None + assert self._params.remote_cfg is not None + + return self.user.vfs.calc_checksum( + checksum_type=self._params.remote_cfg.crc_type, + file_path=self._put_req.source_file, + file_sz=size_to_calculate, + segment_len=self._params.fp.segment_len, + ) diff --git a/tests/test_checksum.py b/tests/test_checksum.py deleted file mode 100644 index 42fae0c..0000000 --- a/tests/test_checksum.py +++ /dev/null @@ -1,66 +0,0 @@ -import os -import struct -from pyfakefs.fake_filesystem_unittest import TestCase -from tempfile import gettempdir -from pathlib import Path -from cfdppy.handler.crc import CrcHelper, calc_modular_checksum -from cfdppy.user import NativeFilestore -from spacepackets.cfdp import ChecksumType - - -EXAMPLE_DATA_CFDP = bytes( - [ - 0x00, - 0x01, - 0x02, - 0x03, - 0x04, - 0x05, - 0x06, - 0x07, - 0x08, - 0x09, - 0x0A, - 0x0B, - 0x0C, - 0x0D, - 0x0E, - ] -) - - -class TestChecksumHelper(TestCase): - def setUp(self): - self.setUpPyfakefs() - self.crc_helper = CrcHelper(ChecksumType.NULL_CHECKSUM, NativeFilestore()) - self.file_path = Path(f"{gettempdir()}/crc_file") - with open(self.file_path, "wb") as file: - file.write(EXAMPLE_DATA_CFDP) - # Kind of re-writing the modular checksum impl here which we are trying to test, but the - # numbers/correctness were verified manually using calculators, so this is okay. - segments_to_add = [] - for i in range(4): - if (i + 1) * 4 > len(EXAMPLE_DATA_CFDP): - data_to_add = EXAMPLE_DATA_CFDP[i * 4 :].ljust(4, bytes([0])) - else: - data_to_add = EXAMPLE_DATA_CFDP[i * 4 : (i + 1) * 4] - segments_to_add.append( - int.from_bytes( - data_to_add, - byteorder="big", - signed=False, - ) - ) - full_sum = sum(segments_to_add) - full_sum %= 2**32 - - self.expected_checksum_for_example = struct.pack("!I", full_sum) - - def test_modular_checksum(self): - self.assertEqual( - calc_modular_checksum(self.file_path), self.expected_checksum_for_example - ) - - def tearDown(self): - if self.file_path.exists(): - os.remove(self.file_path) diff --git a/tests/test_filestore.py b/tests/test_filestore.py index c2f97a6..3b6b1d7 100644 --- a/tests/test_filestore.py +++ b/tests/test_filestore.py @@ -1,10 +1,32 @@ import os.path from pathlib import Path import tempfile +import struct +from cfdppy.crc import calc_modular_checksum from pyfakefs.fake_filesystem_unittest import TestCase from cfdppy.filestore import NativeFilestore, FilestoreResult +EXAMPLE_DATA_CFDP = bytes( + [ + 0x00, + 0x01, + 0x02, + 0x03, + 0x04, + 0x05, + 0x06, + 0x07, + 0x08, + 0x09, + 0x0A, + 0x0B, + 0x0C, + 0x0D, + 0x0E, + ] +) + class TestCfdpHostFilestore(TestCase): def setUp(self): @@ -17,6 +39,29 @@ def setUp(self): self.test_list_dir_name = Path(f"{self.temp_dir}/list-dir-test.txt") self.filestore = NativeFilestore() + self.file_path = Path(f"{tempfile.gettempdir()}/crc_file") + with open(self.file_path, "wb") as file: + file.write(EXAMPLE_DATA_CFDP) + # Kind of re-writing the modular checksum impl here which we are trying to test, but the + # numbers/correctness were verified manually using calculators, so this is okay. + segments_to_add = [] + for i in range(4): + if (i + 1) * 4 > len(EXAMPLE_DATA_CFDP): + data_to_add = EXAMPLE_DATA_CFDP[i * 4 :].ljust(4, bytes([0])) + else: + data_to_add = EXAMPLE_DATA_CFDP[i * 4 : (i + 1) * 4] + segments_to_add.append( + int.from_bytes( + data_to_add, + byteorder="big", + signed=False, + ) + ) + full_sum = sum(segments_to_add) + full_sum %= 2**32 + + self.expected_checksum_for_example = struct.pack("!I", full_sum) + def test_creation(self): res = self.filestore.create_file(self.test_file_name_0) self.assertTrue(res == FilestoreResult.CREATE_SUCCESS) @@ -93,5 +138,11 @@ def test_list_dir(self): ) self.assertTrue(res == FilestoreResult.SUCCESS) + def test_modular_checksum(self): + self.assertEqual( + calc_modular_checksum(self.file_path), self.expected_checksum_for_example + ) + def tearDown(self): - pass + if self.file_path.exists(): + os.remove(self.file_path) diff --git a/tests/test_src_handler.py b/tests/test_src_handler.py index 169dcce..b2357e5 100644 --- a/tests/test_src_handler.py +++ b/tests/test_src_handler.py @@ -20,7 +20,6 @@ from crcmod.predefined import PredefinedCrc from pyfakefs.fake_filesystem_unittest import TestCase from spacepackets.cfdp import ( - NULL_CHECKSUM_U32, ChecksumType, ConditionCode, FinishedParams, @@ -124,7 +123,8 @@ def _common_empty_file_test( closure_requested=None, ) metadata_pdu, transaction_id = self._start_source_transaction(put_req) - eof_pdu = self._handle_eof_pdu(transaction_id, NULL_CHECKSUM_U32, 0) + crc32 = PredefinedCrc("crc32").digest() + eof_pdu = self._handle_eof_pdu(transaction_id, crc32, 0) return transaction_id, metadata_pdu, eof_pdu def _handle_eof_pdu( @@ -236,6 +236,7 @@ def _transaction_with_file_data_wrapper( put_req: PutRequest, data: Optional[bytes], originating_transaction_id: Optional[TransactionId] = None, + crc_type: ChecksumType = ChecksumType.CRC_32, ) -> TransactionStartParams: file_size = None crc32 = None @@ -245,7 +246,7 @@ def _transaction_with_file_data_wrapper( file_size = put_req.source_file.stat().st_size self.local_cfg.local_entity_id = self.source_id metadata_pdu, transaction_id = self._start_source_transaction( - put_req, originating_transaction_id + put_req, originating_transaction_id, crc_type ) self.assertEqual(transaction_id.source_id.value, self.source_id.value) self.assertEqual(transaction_id.seq_num.value, self.expected_seq_num) @@ -284,6 +285,7 @@ def _start_source_transaction( self, put_request: PutRequest, expected_originating_id: Optional[TransactionId] = None, + crc_type: ChecksumType = ChecksumType.CRC_32, ) -> Tuple[MetadataPdu, TransactionId]: self.source_handler.put_request(put_request) fsm_res = self.source_handler.state_machine() @@ -305,7 +307,7 @@ def _start_source_transaction( self.assertEqual( metadata_pdu.params.closure_requested, put_request.closure_requested ) - self.assertEqual(metadata_pdu.checksum_type, ChecksumType.CRC_32) + self.assertEqual(metadata_pdu.checksum_type, crc_type) source_file_as_posix = None if put_request.source_file is not None: source_file_as_posix = put_request.source_file.as_posix() diff --git a/tests/test_src_handler_nak_no_closure.py b/tests/test_src_handler_nak_no_closure.py index cc57812..26213bd 100644 --- a/tests/test_src_handler_nak_no_closure.py +++ b/tests/test_src_handler_nak_no_closure.py @@ -17,6 +17,7 @@ TransactionId, TransmissionMode, ) +from spacepackets.cfdp.defs import ChecksumType from spacepackets.cfdp.pdu import DeliveryCode, FileStatus from spacepackets.cfdp.pdu.finished import FinishedParams from spacepackets.cfdp.tlv import ( @@ -239,7 +240,10 @@ def test_proxy_put_response_no_originating_id(self): self.dest_id = ByteFieldU8(2) self.source_handler.source_id = self.source_id self._transaction_with_file_data_wrapper( - put_req, data=None, originating_transaction_id=None + put_req, + data=None, + originating_transaction_id=None, + crc_type=ChecksumType.NULL_CHECKSUM, ) self.source_handler.state_machine() self._test_transaction_completion()