From 64c75730a34bf7f746bf4fc7f60fc4531953ce08 Mon Sep 17 00:00:00 2001 From: thomas-bc Date: Thu, 2 Nov 2023 17:36:15 -0700 Subject: [PATCH 1/3] Handle PermissionErrors on downlink --- src/fprime_gds/common/files/downlinker.py | 22 ++++++++++++++++------ src/fprime_gds/common/files/helpers.py | 1 + src/fprime_gds/common/pipeline/files.py | 6 ++++++ 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/fprime_gds/common/files/downlinker.py b/src/fprime_gds/common/files/downlinker.py index 452114a0..65aa8041 100644 --- a/src/fprime_gds/common/files/downlinker.py +++ b/src/fprime_gds/common/files/downlinker.py @@ -52,7 +52,6 @@ def __init__(self, directory, timeout=20.0, log_dir=None): self.sequence = 0 # Keeps track of what the current sequence ID should be self.timer = fprime_gds.common.files.helpers.Timeout() self.timer.setup(self.timeout, timeout) - os.makedirs(self.__directory, exist_ok=True) def data_callback(self, data, sender=None): """ @@ -96,7 +95,12 @@ def handle_start(self, data): size, self.__log_dir, ) - self.active.open(TransmitFileState.WRITE) + try: + self.active.open(TransmitFileState.WRITE) + except PermissionError as exc: + self.state = FileStates.ERROR + LOGGER.warning("Unable to open file for writing. Discarding subsequent downlink packets. " + str(exc)) + return LOGGER.addHandler(self.active.log_handler) message = "Received START packet with metadata:\n" message += "\tSize: %d\n" @@ -116,7 +120,10 @@ def handle_data(self, data): # Initialize all relevant DATA packet attributes into variables from file_data offset = data.offset if self.state != FileStates.RUNNING: - LOGGER.warning("Received unexpected data packet for offset: %d", offset) + # ERROR state means GDS is not ready to receive data packets (permission error) + # No need to log this, as it is already logged in handle_start and would only spam logs + if self.state != FileStates.ERROR: + LOGGER.warning("Received unexpected data packet for offset: %d", offset) else: if data.seqID != self.sequence: LOGGER.warning( @@ -156,9 +163,10 @@ def handle_end(self, data): """ # Initialize all relevant END packet attributes into variables from file_data # hashValue attribute is not relevant right now, but might be in the future - if self.state != FileStates.RUNNING: - LOGGER.warning("Received unexpected END packet") - else: + if self.state == FileStates.ERROR: + LOGGER.info("Received END packet for discarded downlink") + self.finish() + elif self.state == FileStates.RUNNING: if data.seqID != self.sequence: LOGGER.warning( "End packet has unexpected sequence id. Expected: %d got %d", @@ -167,6 +175,8 @@ def handle_end(self, data): ) LOGGER.info("Received END packet, finishing downlink") self.finish() + else: + LOGGER.warning("Received unexpected END packet") def timeout(self): """Timeout the current downlink""" diff --git a/src/fprime_gds/common/files/helpers.py b/src/fprime_gds/common/files/helpers.py index ee983615..1b588fc2 100644 --- a/src/fprime_gds/common/files/helpers.py +++ b/src/fprime_gds/common/files/helpers.py @@ -77,6 +77,7 @@ class FileStates(enum.Enum): RUNNING = 1 CANCELED = 2 END_WAIT = 3 # Waiting for the handshake for CANCEL or END packet + ERROR = 4 class CFDPChecksum: diff --git a/src/fprime_gds/common/pipeline/files.py b/src/fprime_gds/common/pipeline/files.py index 43542192..4232ee5c 100644 --- a/src/fprime_gds/common/pipeline/files.py +++ b/src/fprime_gds/common/pipeline/files.py @@ -7,6 +7,7 @@ @author mstarch """ +import os import fprime_gds.common.files.downlinker import fprime_gds.common.files.uplinker @@ -41,6 +42,11 @@ def setup_file_handling( ) file_decoder.register(self.__downlinker) distributor.register("FW_PACKET_HAND", self.__uplinker) + if not os.access(down_store, os.W_OK): + print( + f"[WARNING] {down_store} is not writable. Downlinker will not be able to save files." + " Fix permissions or change storage directory with --file-storage-directory" + ) @property def uplinker(self): From cc9c2fc6da3bc2e779be5d945a7cada79d394112 Mon Sep 17 00:00:00 2001 From: thomas-bc Date: Thu, 2 Nov 2023 17:43:52 -0700 Subject: [PATCH 2/3] spelling and formatting --- .github/actions/spelling/expect.txt | 1 + src/fprime_gds/common/files/downlinker.py | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index d9fe23d0..14e4982f 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -572,6 +572,7 @@ Smath SNDHWM socketserver sorttable +spam sphinxcontrib splitext squashify diff --git a/src/fprime_gds/common/files/downlinker.py b/src/fprime_gds/common/files/downlinker.py index 65aa8041..45beae47 100644 --- a/src/fprime_gds/common/files/downlinker.py +++ b/src/fprime_gds/common/files/downlinker.py @@ -99,7 +99,10 @@ def handle_start(self, data): self.active.open(TransmitFileState.WRITE) except PermissionError as exc: self.state = FileStates.ERROR - LOGGER.warning("Unable to open file for writing. Discarding subsequent downlink packets. " + str(exc)) + LOGGER.warning( + "Unable to open file for writing. Discarding subsequent downlink packets. " + + str(exc) + ) return LOGGER.addHandler(self.active.log_handler) message = "Received START packet with metadata:\n" From 9bcd0e06106a915eb08fe8b6b18a924b3479d5c0 Mon Sep 17 00:00:00 2001 From: thomas-bc Date: Mon, 13 Nov 2023 14:55:13 -0800 Subject: [PATCH 3/3] Raise PermissionError instead of warning --- src/fprime_gds/common/pipeline/files.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/fprime_gds/common/pipeline/files.py b/src/fprime_gds/common/pipeline/files.py index 4232ee5c..f88b475c 100644 --- a/src/fprime_gds/common/pipeline/files.py +++ b/src/fprime_gds/common/pipeline/files.py @@ -28,7 +28,8 @@ def setup_file_handling( self, down_store, file_encoder, file_decoder, distributor, log_dir ): """ - Sets up the file handling (uplink and downlink) from a pair of encoders and decoders + Sets up the file handling (uplink and downlink) from a pair of encoders and decoders. + Raises a PermissionError if the down_store is not writable. :param down_store: downlink storage directory :param file_encoder: file encoder for uplink @@ -43,9 +44,9 @@ def setup_file_handling( file_decoder.register(self.__downlinker) distributor.register("FW_PACKET_HAND", self.__uplinker) if not os.access(down_store, os.W_OK): - print( - f"[WARNING] {down_store} is not writable. Downlinker will not be able to save files." - " Fix permissions or change storage directory with --file-storage-directory" + raise PermissionError( + f"{down_store} is not writable. Downlinker not be able to save files. " + "Fix permissions or change storage directory with --file-storage-directory." ) @property