From abcf1b29b2002e05b8a3974a9bc27f69531b8668 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 24 Feb 2023 14:50:36 +0100 Subject: [PATCH 1/3] execution complete --- src/fsfw/pus/CServiceHealthCommanding.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fsfw/pus/CServiceHealthCommanding.cpp b/src/fsfw/pus/CServiceHealthCommanding.cpp index 57b704fd..49284b73 100644 --- a/src/fsfw/pus/CServiceHealthCommanding.cpp +++ b/src/fsfw/pus/CServiceHealthCommanding.cpp @@ -79,7 +79,7 @@ ReturnValue_t CServiceHealthCommanding::prepareCommand(CommandMessage *message, } case (Subservice::COMMAND_ANNOUNCE_HEALTH): { HealthMessage::setHealthMessage(message, HealthMessage::HEALTH_ANNOUNCE); - break; + return CommandingServiceBase::EXECUTION_COMPLETE; } case (Subservice::COMMAND_ANNOUNCE_HEALTH_ALL): { ReturnValue_t result = iterateHealthTable(true); From f0415a97b1b610e821f44726e75674a2317de135 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 24 Feb 2023 15:49:05 +0100 Subject: [PATCH 2/3] some fixes and improvements --- src/fsfw/cfdp/handler/DestHandler.cpp | 104 ++++++++++++++------- src/fsfw/cfdp/handler/DestHandler.h | 4 +- src/fsfw/cfdp/handler/defs.h | 3 + src/fsfw_hal/linux/spi/ManualCsLockGuard.h | 1 + 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/src/fsfw/cfdp/handler/DestHandler.cpp b/src/fsfw/cfdp/handler/DestHandler.cpp index b4d4af09..1b1187b1 100644 --- a/src/fsfw/cfdp/handler/DestHandler.cpp +++ b/src/fsfw/cfdp/handler/DestHandler.cpp @@ -178,16 +178,25 @@ ReturnValue_t cfdp::DestHandler::handleFileDataPdu(const cfdp::PacketInfo& info) dp.user.fileSegmentRecvdIndication(segParams); } result = dp.user.vfs.writeToFile(fileOpParams, fileData); - if (offset.value() + fileSegmentLen > tp.progress) { - tp.progress = offset.value() + fileSegmentLen; - } if (result != returnvalue::OK) { // TODO: Proper Error handling #if FSFW_CPP_OSTREAM_ENABLED == 1 - sif::error << "File write error" << std::endl; + sif::error << "cfdp::DestHandler: VFS file write error with code 0x" << std::hex << std::setw(2) + << result << std::endl; #endif + tp.vfsErrorCount++; + if (tp.vfsErrorCount < 3) { + // TODO: Provide execution step as parameter + fp.eventReporter->forwardEvent(events::FILESTORE_ERROR, static_cast(fsmRes.step), + result); + } + return result; } else { tp.deliveryStatus = FileDeliveryStatus::RETAINED_IN_FILESTORE; + tp.vfsErrorCount = 0; + } + if (offset.value() + fileSegmentLen > tp.progress) { + tp.progress = offset.value() + fileSegmentLen; } return result; } @@ -271,35 +280,62 @@ ReturnValue_t cfdp::DestHandler::startTransaction(MetadataPduReader& reader, Met return OK; } ReturnValue_t result = OK; - fsmRes.step = TransactionStep::TRANSACTION_START; - if (reader.getTransmissionMode() == TransmissionMode::UNACKNOWLEDGED) { - fsmRes.state = CfdpStates::BUSY_CLASS_1_NACKED; - } else if (reader.getTransmissionMode() == TransmissionMode::ACKNOWLEDGED) { - fsmRes.state = CfdpStates::BUSY_CLASS_2_ACKED; - } - tp.checksumType = info.getChecksumType(); - tp.closureRequested = info.isClosureRequested(); size_t sourceNameSize = 0; const uint8_t* sourceNamePtr = info.getSourceFileName().getValue(&sourceNameSize); - if (sourceNameSize > tp.sourceName.size()) { - // TODO: Warning, event etc. + if (sourceNameSize + 1 > tp.sourceName.size()) { + fp.eventReporter->forwardEvent(events::FILENAME_TOO_LARGE_ERROR, 0, 0); +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "cfdp::DestHandler: source filename too large" << std::endl; +#endif return FAILED; } std::memcpy(tp.sourceName.data(), sourceNamePtr, sourceNameSize); tp.sourceName[sourceNameSize] = '\0'; size_t destNameSize = 0; const uint8_t* destNamePtr = info.getDestFileName().getValue(&destNameSize); - if (destNameSize > tp.destName.size()) { - // TODO: Warning, event etc. + if (destNameSize + 1 > tp.destName.size()) { + fp.eventReporter->forwardEvent(events::FILENAME_TOO_LARGE_ERROR, 0, 0); +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "cfdp::DestHandler: dest filename too large" << std::endl; +#endif return FAILED; } std::memcpy(tp.destName.data(), destNamePtr, destNameSize); tp.destName[destNameSize] = '\0'; - reader.fillConfig(tp.pduConf); - tp.pduConf.direction = Direction::TOWARDS_SENDER; - tp.transactionId.entityId = tp.pduConf.sourceId; - tp.transactionId.seqNum = tp.pduConf.seqNum; - if (not dp.remoteCfgTable.getRemoteCfg(tp.pduConf.sourceId, &tp.remoteCfg)) { + + // If both dest name size and source name size are 0, we are dealing with a metadata only PDU, + // so there is no need to create a file or truncate an existing file + if (destNameSize > 0 and sourceNameSize > 0) { + FilesystemParams fparams(tp.destName.data()); + if (dp.user.vfs.fileExists(fparams)) { + result = dp.user.vfs.truncateFile(fparams); + if (result != returnvalue::OK) { + // TODO: Provide execution step as parameter + fp.eventReporter->forwardEvent(events::FILESTORE_ERROR, static_cast(fsmRes.step), + result); +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "cfdp::DestHandler: file truncation error with code " << result + << std::endl; +#endif + return FAILED; + // TODO: Relevant for filestore rejection error? + } + } else { + result = dp.user.vfs.createFile(fparams); + if (result != OK) { + fp.eventReporter->forwardEvent(events::FILESTORE_ERROR, static_cast(fsmRes.step), + result); +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "cfdp::DestHandler: file creation error with code " << result << std::endl; +#endif + return FAILED; + // TODO: Relevant for filestore rejection error? + } + } + } + EntityId sourceId; + reader.getSourceId(sourceId); + if (not dp.remoteCfgTable.getRemoteCfg(sourceId, &tp.remoteCfg)) { // TODO: Warning, event etc. #if FSFW_CPP_OSTREAM_ENABLED == 1 sif::warning << "cfdp::DestHandler" << __func__ @@ -308,22 +344,18 @@ ReturnValue_t cfdp::DestHandler::startTransaction(MetadataPduReader& reader, Met #endif return FAILED; } - // If both dest name size and source name size are 0, we are dealing with a metadata only PDU, - // so there is no need to create a file or truncate an existing file - if (destNameSize > 0 and sourceNameSize > 0) { - FilesystemParams fparams(tp.destName.data()); - // TODO: Filesystem errors? - if (dp.user.vfs.fileExists(fparams)) { - dp.user.vfs.truncateFile(fparams); - } else { - result = dp.user.vfs.createFile(fparams); - if (result != OK) { - // TODO: Handle FS error. This is probably a case for the filestore rejection mechanism of - // CFDP. - // In any case, it does not really make sense to continue here - } - } + fsmRes.step = TransactionStep::TRANSACTION_START; + if (reader.getTransmissionMode() == TransmissionMode::UNACKNOWLEDGED) { + fsmRes.state = CfdpStates::BUSY_CLASS_1_NACKED; + } else if (reader.getTransmissionMode() == TransmissionMode::ACKNOWLEDGED) { + fsmRes.state = CfdpStates::BUSY_CLASS_2_ACKED; } + tp.checksumType = info.getChecksumType(); + tp.closureRequested = info.isClosureRequested(); + reader.fillConfig(tp.pduConf); + tp.pduConf.direction = Direction::TOWARDS_SENDER; + tp.transactionId.entityId = tp.pduConf.sourceId; + tp.transactionId.seqNum = tp.pduConf.seqNum; fsmRes.step = TransactionStep::RECEIVING_FILE_DATA_PDUS; MetadataRecvdParams params(tp.transactionId, tp.pduConf.sourceId); params.fileSize = tp.fileSize.getSize(); diff --git a/src/fsfw/cfdp/handler/DestHandler.h b/src/fsfw/cfdp/handler/DestHandler.h index 5cef88d4..357493df 100644 --- a/src/fsfw/cfdp/handler/DestHandler.h +++ b/src/fsfw/cfdp/handler/DestHandler.h @@ -84,7 +84,7 @@ enum class CallStatus { DONE, CALL_AFTER_DELAY, CALL_AGAIN }; class DestHandler { public: - enum class TransactionStep { + enum class TransactionStep : uint8_t { IDLE = 0, TRANSACTION_START = 1, RECEIVING_FILE_DATA_PDUS = 2, @@ -157,11 +157,13 @@ class DestHandler { progress = 0; remoteCfg = nullptr; closureRequested = false; + vfsErrorCount = 0; checksumType = ChecksumType::NULL_CHECKSUM; } ChecksumType checksumType = ChecksumType::NULL_CHECKSUM; bool closureRequested = false; + uint16_t vfsErrorCount = 0; std::vector sourceName; std::vector destName; cfdp::FileSize fileSize; diff --git a/src/fsfw/cfdp/handler/defs.h b/src/fsfw/cfdp/handler/defs.h index 190fb67d..5f17ca2d 100644 --- a/src/fsfw/cfdp/handler/defs.h +++ b/src/fsfw/cfdp/handler/defs.h @@ -12,6 +12,9 @@ namespace events { static constexpr Event STORE_ERROR = event::makeEvent(SSID, 0, severity::LOW); static constexpr Event MSG_QUEUE_ERROR = event::makeEvent(SSID, 1, severity::LOW); static constexpr Event SERIALIZATION_ERROR = event::makeEvent(SSID, 2, severity::LOW); +static constexpr Event FILESTORE_ERROR = event::makeEvent(SSID, 3, severity::LOW); +//! [EXPORT] : [COMMENT] P1: Transaction step ID, P2: 0 for source file name, 1 for dest file name +static constexpr Event FILENAME_TOO_LARGE_ERROR = event::makeEvent(SSID, 4, severity::LOW); } // namespace events diff --git a/src/fsfw_hal/linux/spi/ManualCsLockGuard.h b/src/fsfw_hal/linux/spi/ManualCsLockGuard.h index 1f0997b0..8456f9f1 100644 --- a/src/fsfw_hal/linux/spi/ManualCsLockGuard.h +++ b/src/fsfw_hal/linux/spi/ManualCsLockGuard.h @@ -1,6 +1,7 @@ #pragma once #include + #include "fsfw/ipc/MutexIF.h" #include "fsfw/returnvalues/returnvalue.h" #include "fsfw_hal/common/gpio/GpioIF.h" From 893b43472872cdc90b4d602670c8fa9750ca7407 Mon Sep 17 00:00:00 2001 From: Robin Mueller Date: Fri, 24 Feb 2023 16:49:23 +0100 Subject: [PATCH 3/3] allow dest handler to handle folder destinations --- src/fsfw/cfdp/handler/DestHandler.cpp | 64 ++++++++++++++++++--------- src/fsfw/cfdp/handler/DestHandler.h | 2 + src/fsfw/filesystem/HasFileSystemIF.h | 6 +++ src/fsfw_hal/host/HostFilesystem.cpp | 15 +++++++ src/fsfw_hal/host/HostFilesystem.h | 3 ++ unittests/mocks/FilesystemMock.cpp | 7 +++ unittests/mocks/FilesystemMock.h | 4 ++ 7 files changed, 81 insertions(+), 20 deletions(-) diff --git a/src/fsfw/cfdp/handler/DestHandler.cpp b/src/fsfw/cfdp/handler/DestHandler.cpp index 1b1187b1..4ffa797f 100644 --- a/src/fsfw/cfdp/handler/DestHandler.cpp +++ b/src/fsfw/cfdp/handler/DestHandler.cpp @@ -281,12 +281,10 @@ ReturnValue_t cfdp::DestHandler::startTransaction(MetadataPduReader& reader, Met } ReturnValue_t result = OK; size_t sourceNameSize = 0; + const uint8_t* sourceNamePtr = info.getSourceFileName().getValue(&sourceNameSize); if (sourceNameSize + 1 > tp.sourceName.size()) { - fp.eventReporter->forwardEvent(events::FILENAME_TOO_LARGE_ERROR, 0, 0); -#if FSFW_CPP_OSTREAM_ENABLED == 1 - sif::warning << "cfdp::DestHandler: source filename too large" << std::endl; -#endif + fileErrorHandler(events::FILENAME_TOO_LARGE_ERROR, 0, "source filename too large"); return FAILED; } std::memcpy(tp.sourceName.data(), sourceNamePtr, sourceNameSize); @@ -294,10 +292,7 @@ ReturnValue_t cfdp::DestHandler::startTransaction(MetadataPduReader& reader, Met size_t destNameSize = 0; const uint8_t* destNamePtr = info.getDestFileName().getValue(&destNameSize); if (destNameSize + 1 > tp.destName.size()) { - fp.eventReporter->forwardEvent(events::FILENAME_TOO_LARGE_ERROR, 0, 0); -#if FSFW_CPP_OSTREAM_ENABLED == 1 - sif::warning << "cfdp::DestHandler: dest filename too large" << std::endl; -#endif + fileErrorHandler(events::FILENAME_TOO_LARGE_ERROR, 0, "dest filename too large"); return FAILED; } std::memcpy(tp.destName.data(), destNamePtr, destNameSize); @@ -307,27 +302,25 @@ ReturnValue_t cfdp::DestHandler::startTransaction(MetadataPduReader& reader, Met // so there is no need to create a file or truncate an existing file if (destNameSize > 0 and sourceNameSize > 0) { FilesystemParams fparams(tp.destName.data()); + // handling to allow only specifying target directory. Example: + // Source path /test/hello.txt, dest path /tmp -> dest path /tmp/hello.txt + if (dp.user.vfs.isDirectory(tp.destName.data())) { + result = tryBuildingAbsoluteDestName(destNameSize); + if (result != OK) { + return result; + } + } if (dp.user.vfs.fileExists(fparams)) { result = dp.user.vfs.truncateFile(fparams); if (result != returnvalue::OK) { - // TODO: Provide execution step as parameter - fp.eventReporter->forwardEvent(events::FILESTORE_ERROR, static_cast(fsmRes.step), - result); -#if FSFW_CPP_OSTREAM_ENABLED == 1 - sif::warning << "cfdp::DestHandler: file truncation error with code " << result - << std::endl; -#endif + fileErrorHandler(events::FILESTORE_ERROR, result, "file truncation error"); return FAILED; // TODO: Relevant for filestore rejection error? } } else { result = dp.user.vfs.createFile(fparams); if (result != OK) { - fp.eventReporter->forwardEvent(events::FILESTORE_ERROR, static_cast(fsmRes.step), - result); -#if FSFW_CPP_OSTREAM_ENABLED == 1 - sif::warning << "cfdp::DestHandler: file creation error with code " << result << std::endl; -#endif + fileErrorHandler(events::FILESTORE_ERROR, result, "file creation error"); return FAILED; // TODO: Relevant for filestore rejection error? } @@ -394,6 +387,37 @@ ReturnValue_t cfdp::DestHandler::handleTransferCompletion() { return OK; } +ReturnValue_t cfdp::DestHandler::tryBuildingAbsoluteDestName(size_t destNameSize) { + char baseNameBuf[tp.destName.size()]{}; + FilesystemParams fparamsSrc(tp.sourceName.data()); + size_t baseNameLen = 0; + ReturnValue_t result = + dp.user.vfs.getBaseFilename(fparamsSrc, baseNameBuf, sizeof(baseNameBuf), baseNameLen); + if (result != returnvalue::OK or baseNameLen == 0) { + fileErrorHandler(events::FILENAME_TOO_LARGE_ERROR, 0, "error retrieving source base name"); + return FAILED; + } + // Destination name + slash + base name + null termination + if (destNameSize + 1 + baseNameLen + 1 > tp.destName.size()) { + fileErrorHandler(events::FILENAME_TOO_LARGE_ERROR, 0, + "dest filename too large after adding source base name"); + return FAILED; + } + tp.destName[destNameSize++] = '/'; + std::memcpy(tp.destName.data() + destNameSize, baseNameBuf, baseNameLen); + destNameSize += baseNameLen; + tp.destName[destNameSize++] = '\0'; + return OK; +} + +void cfdp::DestHandler::fileErrorHandler(Event event, ReturnValue_t result, const char* info) { + fp.eventReporter->forwardEvent(events::FILENAME_TOO_LARGE_ERROR, + static_cast(fsmRes.step), result); +#if FSFW_CPP_OSTREAM_ENABLED == 1 + sif::warning << "cfdp::DestHandler: " << info << std::endl; +#endif +} + void cfdp::DestHandler::finish() { tp.reset(); dp.packetListRef.clear(); diff --git a/src/fsfw/cfdp/handler/DestHandler.h b/src/fsfw/cfdp/handler/DestHandler.h index 357493df..9057b3f5 100644 --- a/src/fsfw/cfdp/handler/DestHandler.h +++ b/src/fsfw/cfdp/handler/DestHandler.h @@ -191,9 +191,11 @@ class DestHandler { ReturnValue_t handleMetadataParseError(ReturnValue_t result, const uint8_t* rawData, size_t maxSize); ReturnValue_t handleTransferCompletion(); + ReturnValue_t tryBuildingAbsoluteDestName(size_t destNameSize); ReturnValue_t sendFinishedPdu(); ReturnValue_t noticeOfCompletion(); ReturnValue_t checksumVerification(); + void fileErrorHandler(Event event, ReturnValue_t result, const char* info); const FsmResult& updateFsmRes(uint8_t errors); void checkAndHandleError(ReturnValue_t result, uint8_t& errorIdx); void finish(); diff --git a/src/fsfw/filesystem/HasFileSystemIF.h b/src/fsfw/filesystem/HasFileSystemIF.h index 24400b1c..a507938e 100644 --- a/src/fsfw/filesystem/HasFileSystemIF.h +++ b/src/fsfw/filesystem/HasFileSystemIF.h @@ -74,6 +74,12 @@ class HasFileSystemIF { return MessageQueueIF::NO_QUEUE; } + // Get the base filename without the full directory path + virtual ReturnValue_t getBaseFilename(FilesystemParams params, char* nameBuf, size_t maxLen, + size_t& baseNameLen) = 0; + + virtual bool isDirectory(const char* path) = 0; + virtual bool fileExists(FilesystemParams params) = 0; /** diff --git a/src/fsfw_hal/host/HostFilesystem.cpp b/src/fsfw_hal/host/HostFilesystem.cpp index fe593f27..d430d0f0 100644 --- a/src/fsfw_hal/host/HostFilesystem.cpp +++ b/src/fsfw_hal/host/HostFilesystem.cpp @@ -160,3 +160,18 @@ ReturnValue_t HostFilesystem::truncateFile(FilesystemParams params) { ofstream of(path); return returnvalue::OK; } + +bool HostFilesystem::isDirectory(const char *path) { return filesystem::is_directory(path); } + +ReturnValue_t HostFilesystem::getBaseFilename(FilesystemParams params, char *nameBuf, size_t maxLen, + size_t &baseNameLen) { + std::string path(params.path); + std::string baseName = path.substr(path.find_last_of("/\\") + 1); + if (baseName.size() + 1 > maxLen) { + return returnvalue::FAILED; + } + std::memcpy(nameBuf, baseName.c_str(), baseName.size()); + nameBuf[baseName.size()] = '\0'; + baseNameLen = baseName.size(); + return returnvalue::OK; +} diff --git a/src/fsfw_hal/host/HostFilesystem.h b/src/fsfw_hal/host/HostFilesystem.h index 7b865e2d..da217aec 100644 --- a/src/fsfw_hal/host/HostFilesystem.h +++ b/src/fsfw_hal/host/HostFilesystem.h @@ -9,6 +9,9 @@ class HostFilesystem : public HasFileSystemIF { public: HostFilesystem(); + ReturnValue_t getBaseFilename(FilesystemParams params, char *nameBuf, size_t maxLen, + size_t &baseNameLen) override; + bool isDirectory(const char *path) override; bool fileExists(FilesystemParams params) override; ReturnValue_t truncateFile(FilesystemParams params) override; ReturnValue_t writeToFile(FileOpParams params, const uint8_t *data) override; diff --git a/unittests/mocks/FilesystemMock.cpp b/unittests/mocks/FilesystemMock.cpp index bf0c3bf6..24850227 100644 --- a/unittests/mocks/FilesystemMock.cpp +++ b/unittests/mocks/FilesystemMock.cpp @@ -138,3 +138,10 @@ ReturnValue_t FilesystemMock::truncateFile(FilesystemParams params) { truncateCalledOnFile = params.path; return returnvalue::OK; } + +ReturnValue_t FilesystemMock::getBaseFilename(FilesystemParams params, char *nameBuf, size_t maxLen, + size_t &baseNameLen) { + return returnvalue::OK; +} + +bool FilesystemMock::isDirectory(const char *path) { return false; } diff --git a/unittests/mocks/FilesystemMock.h b/unittests/mocks/FilesystemMock.h index 74221d70..2ddbefc3 100644 --- a/unittests/mocks/FilesystemMock.h +++ b/unittests/mocks/FilesystemMock.h @@ -56,6 +56,10 @@ class FilesystemMock : public HasFileSystemIF { std::string truncateCalledOnFile; ReturnValue_t feedFile(const std::string &filename, std::ifstream &file); + ReturnValue_t getBaseFilename(FilesystemParams params, char *nameBuf, size_t maxLen, + size_t &baseNameLen) override; + + bool isDirectory(const char *path) override; bool fileExists(FilesystemParams params) override; ReturnValue_t truncateFile(FilesystemParams params) override;