Work/dp catalog #2713

timcanham · 2024-05-01T17:46:57Z


*Related Issue(s)*
*Has Unit Tests (y/n)*
*Documentation Included (y/n)*

Change Description

A description of the changes contained in the PR.

Rationale

A rationale for this change. e.g. fixes bug, or most projects need XYZ feature.

Testing/Review Recommendations

Fill in testing procedures, specific items to focus on for review, or other info to help the team verify these changes are flight-quality.

Future Work

Note any additional work that will be done relating to this issue.

Svc/DpCatalog/DpCatalog.cpp

github-advanced-security

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Svc/DpCatalog/DpCatalog.cpp

Svc/DpCatalog/DpCatalog.hpp

config/DpCfg.hpp

Svc/DpCatalog/DpCatalog.cpp

LeStarch · 2024-12-11T00:48:08Z

Svc/DpCatalog/DpCatalog.cpp

+            }
+
+            // reset the buffer for deserializing the entry
+            entryBuffer.setBuffLen(static_cast<Fw::Serializable::SizeType>(size));


Probably should address this one.

LeStarch · 2024-12-11T00:48:40Z

Svc/DpCatalog/DpCatalog.cpp

+                // reset the buffer for serializing the entry
+                entryBuffer.resetSer();
+                // serialize the file directory index
+                entryBuffer.serialize(this->m_stateFileData[entry].entry.dir);


LeStarch · 2024-12-11T00:39:43Z

Svc/DpCatalog/DpCatalog.cpp

+                entryBuffer.resetSer();
+                // serialize the file directory index
+                entryBuffer.serialize(this->m_stateFileData[entry].entry.dir);
+                entryBuffer.serialize(this->m_stateFileData[entry].entry.record);


This is a must fix.

Svc/DpCatalog/DpCatalog.cpp

LeStarch

Commenting partial review.

LeStarch · 2024-12-05T02:40:00Z

Ref/SignalGen/SignalGen.cpp

@@ -134,15 +135,22 @@ namespace Ref {

        // if a Data product is being generated, store a record
        if (this->m_dpInProgress) {
+            // printf("DP1: %u %u %lu %u\n",


LeStarch · 2024-12-05T02:40:12Z

Ref/SignalGen/SignalGen.cpp

            Fw::SerializeStatus stat = this->m_dpContainer.serializeRecord_DataRecord(sigInfo);
            this->m_currDp++;
            this->m_dpBytes += SignalInfo::SERIALIZED_SIZE;
+            // printf("DP2: %u %u %lu\n",this->m_dpContainer.getBuffer().getSize(),this->m_dpBytes,this->m_dpContainer.getDataSize());


LeStarch · 2024-12-05T02:42:49Z

Ref/SignalGen/test/ut/SignalGenTester.cpp

@@ -41,7 +41,7 @@ namespace Ref {
    test_start()
  {
       ASSERT_TLM_Output_SIZE(0);
-       sendCmd_SignalGen_Toggle(0, 0);
+       sendCmd_Toggle(0, 0);


No UT for data product command?

LeStarch · 2024-12-05T02:44:46Z

config/DpCfg.hpp

@@ -17,6 +17,7 @@

 // The format string for a file name
 // The format arguments are base directory, container ID, time seconds, and time microseconds
-constexpr const char *DP_FILENAME_FORMAT = "%s/Dp_%08" PRI_FwDpIdType "_%08" PRIu32 "_%08" PRIu32 ".fdp";
+#define DP_EXT ".fdp"


Why call this out when the user could edit below?

LeStarch · 2024-12-05T02:46:17Z

Svc/DpCatalog/test/ut/seq/Makefile

@@ -0,0 +1,12 @@
+


Don't check in makefiles.

LeStarch · 2024-12-05T03:30:10Z

Svc/DpCatalog/DpCatalog.cpp

+
+        // buffer for reading entries
+
+        BYTE buffer[sizeof(FwIndexType)+DpRecord::SERIALIZED_SIZE];


maybe use sizeof(entry.dir) so it is auto-calculated?

LeStarch · 2024-12-05T03:32:49Z

Svc/DpCatalog/DpCatalog.cpp

+        }
+
+        // buffer for writing entries
+        BYTE buffer[sizeof(FwIndexType)+DpRecord::SERIALIZED_SIZE];


LeStarch · 2024-12-05T03:33:44Z

Svc/DpCatalog/DpCatalog.cpp

+        FW_ASSERT(this->m_stateFileData);
+
+        // We will append state to the existing state file
+        // FIXME: Have to handle case where state file has partially transmitted


LeStarch · 2024-12-05T03:34:14Z

Svc/DpCatalog/DpCatalog.cpp

+        }
+
+        // buffer for writing entries
+        BYTE buffer[sizeof(FwIndexType)+DpRecord::SERIALIZED_SIZE];


Make this a constant, the same expression is used everywhere.

LeStarch · 2024-12-05T03:34:44Z

Svc/DpCatalog/DpCatalog.cpp

+        BYTE buffer[sizeof(FwIndexType)+DpRecord::SERIALIZED_SIZE];
+        Fw::ExternalSerializeBuffer entryBuffer(buffer, sizeof(buffer));
+        // reset the buffer for serializing the entry
+        entryBuffer.resetSer();


Pull this code into a helper? I see the same few lines repeated.

LeStarch · 2024-12-05T17:42:01Z

Svc/DpCatalog/DpCatalog.cpp

+        // fill the binary tree with DP files
+        response = this->fillBinaryTree();
+        if (response != Fw::CmdResponse::OK) {
+            return response;


Does this cause an inconsistent state? You may have failed with a partial binary tree, and an old or empty state file. This return bypasses the state file prune and write in the next (nominal) step.

bocchino

Looks good to me! Here are my comments.

Not Blocking for This PR

In configure, the partitioning of memory into three pieces uses reinterpret cast on the address of an out-of-bounds array element, and it may not respect alignment requirements. This kind of code should do array access on the underlying byte buffer (so it's in bounds), it should use placement new to create the arrays, and it should respect alignment requirements.
There aren't enough assertions, particularly before pointer dereference ->.

To Fix for this PR If Possible

Add comments to configure to explain what is going on: (a) We are sizing a triple of arrays as an array of triples. (b) We are using array indexing to compute the address after the end of the array.
For consistency, remove the explicit close operation in loadStateFile. In two other places the implicit operation is used.

LeStarch · 2024-12-11T00:43:36Z

Svc/DpCatalog/DpCatalog.cpp

@@ -102,10 +127,233 @@
        this->m_initialized = true;
    }

+    void DpCatalog::resetBinaryTree() {


This is valid.

Svc/DpCatalog/DpCatalog.cpp

LeStarch · 2024-12-11T00:43:09Z

Svc/DpCatalog/DpCatalog.cpp

-
-        // request memory for catalog
-        this->m_memSize = DP_MAX_FILES * (sizeof(DpStateEntry) + sizeof(DpSortedList));
+        this->m_stateFile = stateFile;


Should probably consider this one.

Svc/DpCatalog/DpCatalog.cpp

LeStarch

Nothing is a blocking fix, but we do need fixes eventually.

LeStarch · 2024-12-11T00:43:09Z

Svc/DpCatalog/DpCatalog.cpp

-
-        // request memory for catalog
-        this->m_memSize = DP_MAX_FILES * (sizeof(DpStateEntry) + sizeof(DpSortedList));
+        this->m_stateFile = stateFile;


Should probably consider this one.

LeStarch · 2024-12-11T00:43:36Z

Svc/DpCatalog/DpCatalog.cpp

@@ -102,10 +127,233 @@
        this->m_initialized = true;
    }

+    void DpCatalog::resetBinaryTree() {


This is valid.

Svc/DpCatalog/DpCatalog.cpp

LeStarch · 2024-12-11T00:48:08Z

Svc/DpCatalog/DpCatalog.cpp

+            }
+
+            // reset the buffer for deserializing the entry
+            entryBuffer.setBuffLen(static_cast<Fw::Serializable::SizeType>(size));


Probably should address this one.

LeStarch · 2024-12-11T00:48:40Z

Svc/DpCatalog/DpCatalog.cpp

+                // reset the buffer for serializing the entry
+                entryBuffer.resetSer();
+                // serialize the file directory index
+                entryBuffer.serialize(this->m_stateFileData[entry].entry.dir);


LeStarch · 2024-12-11T00:49:28Z

Svc/DpCatalog/DpCatalog.cpp


                // insert entry into sorted list. if can't insert, quit
-                if (not this->insertEntry(this->m_dpList[this->m_numDpRecords])) {
+                if (not this->insertEntry(entry)) {


Consider fixing this.

LeStarch · 2024-12-11T00:50:03Z

Svc/DpCatalog/DpCatalog.cpp

+
+        // if the tree is empty, add the first entry
+        if (this->m_dpTree == nullptr) {
+            this->allocateNode(this->m_dpTree,entry);


Must fix. Specifically since it is your return value.

LeStarch · 2024-12-11T00:50:45Z

Svc/DpCatalog/DpCatalog.cpp

+    DpCatalog::CheckStat DpCatalog::checkLeftRight(bool condition, DpBtreeNode* &node, const DpStateEntry& newEntry) {
+        if (condition) {
+            if (node->left == nullptr) {
+                if (!this->allocateNode(node->left,newEntry)) {


LeStarch · 2024-12-11T00:50:51Z

Svc/DpCatalog/DpCatalog.cpp

+            }
+        } else {
+            if (node->right == nullptr) {
+                if (!this->allocateNode(node->right,newEntry)) {


matt392code · 2025-01-01T03:22:44Z

Missing return value checks:
dp-catalog-checks.txt
This solution:

Adds checks for buffer operations:
-Buffer length setting
-Serialization operations
-Memory allocation
Provides error logging:
-Specific error codes
-Warning messages
-Error type enumeration
Proper error handling:
-Returns appropriate error responses
-Maintains system state
-Prevents undefined behavior
Key improvements:
-No more unchecked return values
-Clear error reporting
-Better system reliability

timcanham added 30 commits January 19, 2024 13:11

Starting on FDD for DpCatalog

26c8765

More DpCatalog SDD edits

041a450

More refined DpCatalog requirements

c4f6294

Starting prototype code

91a363e

Working on DpCatalog build command

3e7d121

More DpCatalog work

e1a5783

More DpCatalog work

fe96b1a

Merge from devel

666e77b

More work on reading DP headers

f99d851

More work...

4394336

Continued work

f3af664

Spelling fixes

c17652f

More spelling fixes

06c3fa6

First run of init unit test

c80d844

One DP works

2848241

Refactor unit test helpers

7b9be73

Working on multiple DPs

e51a07e

More work on multiple DPs

da0700e

Fixed filesRead bug

0a1176d

Merge from fprime devel

7425848

More merge fixes

1e3452f

Adding DPs to SignalGen

cdd6b71

Merge from F Prime devel

1f0d4ec

Adding DP components

a0748fe

Added DP directory to DpWriter

7753fdd

First working version of Data Products!

977a6a6

Added missing FileDownlink connection

d4a3a0e

Added early proto warning to SDD.

3797067

Switched to Fw::FileNameString

dc5dd7b

Fixed unit test compile

f8ac1cc

timcanham added 2 commits November 6, 2024 20:22

Merge remote-tracking branch 'nasa/devel' into work/DpCatalog

31da9b9

Some compile fixes

f101d8a

github-advanced-security bot found potential problems Nov 7, 2024

View reviewed changes

timcanham added 2 commits November 13, 2024 09:23

Adding more test cases

e1bc0e3

Fixed downlink bug

2fae6ec

github-advanced-security bot found potential problems Nov 13, 2024

View reviewed changes

timcanham added 4 commits November 13, 2024 16:55

Fix SignalGen unit test

3bd7436

Updated SDD and some CI error fixes

d836bda

Fix SDD typo

90cf037

Merge with F Prime devel

84889de

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

Svc/DpCatalog/DpCatalog.cpp Dismissed Show dismissed Hide dismissed

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

Working on docs

07bd01b

timcanham marked this pull request as ready for review December 4, 2024 23:02

LeStarch reviewed Dec 5, 2024

View reviewed changes

bocchino self-requested a review December 5, 2024 18:49

bocchino approved these changes Dec 5, 2024

View reviewed changes

Docs/comments updates

0e1329c

github-advanced-security bot found potential problems Dec 11, 2024

View reviewed changes

Fix typo

3dc293e

github-advanced-security bot found potential problems Dec 11, 2024

View reviewed changes

LeStarch approved these changes Dec 11, 2024

View reviewed changes

LeStarch merged commit 05b817f into nasa:devel Dec 11, 2024
32 of 35 checks passed

LeStarch mentioned this pull request Dec 11, 2024

Address Issues With DpCatalog #3072

Open

LeStarch added the Update Instructions Needed Need to add instructions in the release notes for updates. label Dec 11, 2024

This was referenced Dec 11, 2024

Fix CMake CI and update workflow triggers #3073

Merged

Fix SignalGen commands in integration tests #3075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work/dp catalog #2713

Work/dp catalog #2713

timcanham commented May 1, 2024

github-advanced-security bot left a comment

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch left a comment

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

LeStarch Dec 5, 2024

bocchino left a comment •

edited

Loading

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch left a comment

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

LeStarch Dec 11, 2024

matt392code commented Jan 1, 2025


		// buffer for reading entries

		BYTE buffer[sizeof(FwIndexType)+DpRecord::SERIALIZED_SIZE];

Work/dp catalog #2713

Work/dp catalog #2713

Conversation

timcanham commented May 1, 2024

Change Description

Rationale

Testing/Review Recommendations

Future Work

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocchino left a comment • edited Loading

Choose a reason for hiding this comment

Not Blocking for This PR

To Fix for this PR If Possible

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeStarch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matt392code commented Jan 1, 2025

bocchino left a comment •

edited

Loading