Skip to content

Commit

Permalink
Fix #1944, use 0 as the invalid CFE_SB_MsgId_t value
Browse files Browse the repository at this point in the history
Historically the CFE_SB_MSG_ID_INVALID constant was defined as 0xFFFFFFFF/-1,
where 0 was considered valid.

Although 0 is indeed valid for the first word of a CCSDS primary spacepacket header,
it is not actually valid for use with SB.  Because SB is now more decoupled from CCSDS
header definitions, there are a number of advantages to using 0 instead of -1, as
it is more passively safe:

- Objects which are cleared as part of normal BSS clearing will be set invalid
- Objects which are memset to zero will be set invalid

In contrast, when the invalid value is nonzero, objects which are memset/cleared
are valid by default, and must be actively set invalid to be safe.
  • Loading branch information
jphickey committed Sep 23, 2021
1 parent 168181f commit 7320f2f
Show file tree
Hide file tree
Showing 5 changed files with 14 additions and 14 deletions.
2 changes: 1 addition & 1 deletion modules/cfe_testcase/src/msg_api_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ void TestMsgApiBasic(void)
bool _returned = false;

memset(&cmd, 0xFF, sizeof(cmd));
msgId = CFE_SB_ValueToMsgId(0);
msgId = CFE_SB_ValueToMsgId(1);

/* test msg-init */
UtAssert_INT32_EQ(CFE_MSG_Init(NULL, msgId, sizeof(cmd)), CFE_MSG_BAD_ARGUMENT);
Expand Down
4 changes: 2 additions & 2 deletions modules/core_api/fsw/inc/cfe_sb_api_typedefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@
* This rvalue macro can be used for static/compile-time data initialization to ensure that
* the initialized value does not alias to a valid MsgId object.
*/
#define CFE_SB_MSGID_RESERVED CFE_SB_MSGID_WRAP_VALUE(-1)
#define CFE_SB_MSGID_RESERVED CFE_SB_MSGID_WRAP_VALUE(0)

/**
* \brief A literal of the CFE_SB_MsgId_t type representing an invalid ID
Expand All @@ -109,7 +109,7 @@
* purposes (rvalue), #CFE_SB_MSGID_RESERVED should be used instead.
* However, in the current implementation, they are equivalent.
*/
#define CFE_SB_INVALID_MSG_ID CFE_SB_MSGID_C(-1)
#define CFE_SB_INVALID_MSG_ID CFE_SB_MSGID_C(0)

/**
* \brief Cast/Convert a generic CFE_ResourceId_t to a CFE_SB_PipeId_t
Expand Down
2 changes: 1 addition & 1 deletion modules/msg/ut-coverage/test_cfe_msg_msgid_shared.c
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ void Test_MSG_GetTypeFromMsgId(void)
UtAssert_INT32_EQ(Test_MSG_NotZero(&msg), 0);

UtPrintf("Bad parameter tests, Invalid message ID");
UtAssert_INT32_EQ(CFE_MSG_GetTypeFromMsgId(CFE_SB_INVALID_MSG_ID, &actual), CFE_MSG_BAD_ARGUMENT);
UtAssert_INT32_EQ(CFE_MSG_GetTypeFromMsgId(CFE_SB_ValueToMsgId(-1), &actual), CFE_MSG_BAD_ARGUMENT);

UtPrintf("Set to all F's, test cmd and tlm");
memset(&msg, 0xFF, sizeof(msg));
Expand Down
6 changes: 3 additions & 3 deletions modules/msg/ut-coverage/test_cfe_msg_msgid_v1.c
Original file line number Diff line number Diff line change
Expand Up @@ -56,14 +56,14 @@ void Test_MSG_MsgId(void)
UtAssert_INT32_EQ(CFE_MSG_SetMsgId(&msg, CFE_SB_ValueToMsgId(0xFFFF)), CFE_MSG_BAD_ARGUMENT);
UtAssert_INT32_EQ(Test_MSG_NotZero(&msg), 0);

UtPrintf("Set msg to all F's, set msgid to 0 and verify");
UtPrintf("Set msg to all F's, set msgid to 1 and verify");
memset(&msg, 0xFF, sizeof(msg));
CFE_UtAssert_SUCCESS(CFE_MSG_GetMsgId(&msg, &msgid));
UtAssert_INT32_EQ(CFE_SB_MsgIdToValue(msgid), 0xFFFF);
CFE_UtAssert_SUCCESS(CFE_MSG_SetMsgId(&msg, CFE_SB_ValueToMsgId(0)));
CFE_UtAssert_SUCCESS(CFE_MSG_SetMsgId(&msg, CFE_SB_ValueToMsgId(1)));
UT_DisplayPkt(&msg, sizeof(msg));
CFE_UtAssert_SUCCESS(CFE_MSG_GetMsgId(&msg, &msgid));
UtAssert_INT32_EQ(CFE_SB_MsgIdToValue(msgid), 0);
UtAssert_INT32_EQ(CFE_SB_MsgIdToValue(msgid), 1);
UtAssert_INT32_EQ(Test_MSG_NotF(&msg), MSG_HDRVER_FLAG | MSG_APID_FLAG | MSG_TYPE_FLAG | MSG_HASSEC_FLAG);

UtPrintf("Set msg to all 0, set msgid to max and verify");
Expand Down
14 changes: 7 additions & 7 deletions modules/sb/ut-coverage/sb_UT.c
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ const CFE_SB_MsgId_t SB_UT_LAST_VALID_MID = CFE_SB_MSGID_WRAP_VALUE(CFE_PLATFORM
* This is a "borderline" value to test the limits of the validity checking
* The specific value depends on how MsgId is actually defined internally
*/
const CFE_SB_MsgId_t SB_UT_FIRST_VALID_MID = CFE_SB_MSGID_WRAP_VALUE(0);
const CFE_SB_MsgId_t SB_UT_FIRST_VALID_MID = CFE_SB_MSGID_WRAP_VALUE(1);

/*
* A MsgId value which is in the middle of the valid range
Expand Down Expand Up @@ -1480,15 +1480,15 @@ void Test_SB_Cmds_SendPrevSubs(void)
NumEvts = 2; /* one for each pipe create */

/* Two full pkts to be sent plus five entries in a partial pkt, skipping MSGID 0x0D */
for (i = 0; i < CFE_SB_SUB_ENTRIES_PER_PKT * 2 + 5; i++)
for (i = 1; i < (CFE_SB_SUB_ENTRIES_PER_PKT * 2) + 6; i++)
{
/* Skip subscribing to ALLSUBS mid. This is the one we are testing.
* MsgID for this in CCSDS v.1 was 0x180d so this MID did not appear in the
* SB sub list. This results in multiple NO_SUBS_EID sent which we are not
* testing here so it is irrelevant
* For CCSDS v.2 CFE_SB_ALLSUBS_TLM_MID (0x0d) now appears in the
* SB subscription list and thus we must skip adding 0x0D to the list
* as we were going from MSGID 0-45 (0x00-0x2D)
* as we were going from MSGID 1-46 (0x01-0x2E)
* */
if (i != CFE_SB_ALLSUBS_TLM_MID)
{
Expand Down Expand Up @@ -1533,7 +1533,7 @@ void Test_SB_Cmds_SendPrevSubs(void)
/* Round out the number to three full pkts in order to test branch path
* coverage, MSGID 0x0D was skipped in previous subscription loop
*/
for (; i < CFE_SB_SUB_ENTRIES_PER_PKT * 3; i++)
for (; i < (CFE_SB_SUB_ENTRIES_PER_PKT * 3) + 1; i++)
{
CFE_UtAssert_SETUP(CFE_SB_Subscribe(CFE_SB_ValueToMsgId(i), PipeId1));
NumEvts += 1;
Expand Down Expand Up @@ -2571,18 +2571,18 @@ void Test_Subscribe_MaxMsgIdCount(void)
{
if (i < CFE_PLATFORM_SB_MAX_MSG_IDS)
{
CFE_UtAssert_SETUP(CFE_SB_Subscribe(CFE_SB_ValueToMsgId(i), PipeId2));
CFE_UtAssert_SETUP(CFE_SB_Subscribe(CFE_SB_ValueToMsgId(1 + i), PipeId2));
}
else
{
UtAssert_INT32_EQ(CFE_SB_Subscribe(CFE_SB_ValueToMsgId(i), PipeId2), CFE_SB_MAX_MSGS_MET);
UtAssert_INT32_EQ(CFE_SB_Subscribe(CFE_SB_ValueToMsgId(1 + i), PipeId2), CFE_SB_MAX_MSGS_MET);
}
}

UtAssert_UINT32_EQ(CFE_SB_Global.StatTlmMsg.Payload.PeakMsgIdsInUse, CFE_PLATFORM_SB_MAX_MSG_IDS);
CFE_UtAssert_EVENTSENT(CFE_SB_MAX_MSGS_MET_EID);

CFE_UtAssert_SUCCESS(CFE_SB_Subscribe(CFE_SB_ValueToMsgId(0), PipeId1));
CFE_UtAssert_SUCCESS(CFE_SB_Subscribe(CFE_SB_ValueToMsgId(1), PipeId1));
UtAssert_UINT32_EQ(CFE_SB_Global.StatTlmMsg.Payload.PeakMsgIdsInUse, CFE_PLATFORM_SB_MAX_MSG_IDS);

CFE_UtAssert_TEARDOWN(CFE_SB_DeletePipe(PipeId0));
Expand Down

0 comments on commit 7320f2f

Please sign in to comment.