Skip to content

Commit

Permalink
Fix nasa#40, update traverse history/write file
Browse files Browse the repository at this point in the history 
- rename the function to better indicate what it does
- do not discard the part of the output that has EID/TSN/CC information
- do not pass the return value of snprintf directly to write(), use strlen()
- simplify the code
  • Loading branch information
@jphickey
jphickey committed Jan 12, 2022
1 parent c0bf0bd commit 1fd43a3
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 51 deletions.
41 changes: 26 additions & 15 deletions fsw/src/cf_utils.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,13 @@ CF_Transaction_t *CF_FindTransactionBySequenceNumber(CF_Channel_t *c, CF_Transac

/*----------------------------------------------------------------
*
* Function: CF_TraverseHistory
* Function: CF_Traverse_WriteFile
*
* Application-scope internal function
* See description in cf_utils.h for argument/return detail
*
*-----------------------------------------------------------------*/
int CF_TraverseHistory(CF_CListNode_t *n, CF_Traverse_WriteFileArg_t *context)
int CF_Traverse_WriteFile(CF_CListNode_t *n, CF_Traverse_WriteFileArg_t *context)
{
static const char *dstr[] = {"RX", "TX"};
int i;
Expand All @@ -190,19 +190,30 @@ int CF_TraverseHistory(CF_CListNode_t *n, CF_Traverse_WriteFileArg_t *context)
CF_History_t *h = container_of(n, CF_History_t, cl_node);

CF_Assert(h->dir < CF_Direction_NUM);
len = snprintf(linebuf, sizeof(linebuf) - 1, "SEQ (%d, %d)\tDIR: %s\tPEER %d\tCC: %d", h->src_eid, h->seq_num,
dstr[h->dir], h->peer_eid, h->cc);
for (i = 0; i < 2; ++i)
for (i = 0; i < 3; ++i)
{
static const char *fstr[] = {"SRC: %s", "DST: %s"};
const char *fnames[] = {h->fnames.src_filename, h->fnames.dst_filename};
len = snprintf(linebuf, sizeof(linebuf) - 1, fstr[i], fnames[i]);
ret = CF_WrappedWrite(context->fd, linebuf, len);
switch (i)
{
case 0:
snprintf(linebuf, sizeof(linebuf) - 1, "SEQ (%lu, %lu)\tDIR: %s\tPEER %lu\tCC: %u\t",
(unsigned long)h->src_eid, (unsigned long)h->seq_num, dstr[h->dir], (unsigned long)h->peer_eid,
(unsigned int)h->cc);
break;
case 1:
snprintf(linebuf, sizeof(linebuf) - 1, "SRC: %s\t", h->fnames.src_filename);
break;
case 2:
snprintf(linebuf, sizeof(linebuf) - 1, "DST: %s\n", h->fnames.dst_filename);
break;
}

len = strlen(linebuf);
ret = CF_WrappedWrite(context->fd, linebuf, len);
if (ret != len)
{
context->result = 1; /* failed */
CFE_EVS_SendEvent(CF_EID_ERR_CMD_WHIST_WRITE, CFE_EVS_EventType_ERROR,
"CF: writing queue file failed, expected 0x%08x got 0x%08x", len, ret);
"CF: writing queue file failed, expected %ld got %ld", (long)len, (long)ret);
return CF_CLIST_EXIT;
}
}
Expand All @@ -222,12 +233,12 @@ int CF_TraverseTransactions(CF_CListNode_t *n, CF_Traverse_WriteFileArg_t *conte
{
CF_Transaction_t *t = container_of(n, CF_Transaction_t, cl_node);

/* use CF_TraverseHistory to print filenames and direction */
/* NOTE: ok to ignore return value of CF_TraverseHistory. We care
/* use CF_Traverse_WriteFile to print filenames and direction */
/* NOTE: ok to ignore return value of CF_Traverse_WriteFile. We care
* about the value in context->result. The reason for this confusion
* is CF_TraverseHistory is also a list traversal function. In this
* is CF_Traverse_WriteFile is also a list traversal function. In this
* function we are just calling it directly. */
/* ignore return value */ CF_TraverseHistory(&t->history->cl_node, context);
/* ignore return value */ CF_Traverse_WriteFile(&t->history->cl_node, context);
if (context->result)
{
return CF_CLIST_EXIT;
Expand Down Expand Up @@ -262,7 +273,7 @@ int32 CF_WriteQueueDataToFile(int32 fd, CF_Channel_t *c, CF_QueueIdx_t q)
int32 CF_WriteHistoryQueueDataToFile(int32 fd, CF_Channel_t *c, CF_Direction_t dir)
{
CF_Traverse_WriteFileArg_t arg = {fd, 0, 0};
CF_CList_Traverse(c->qs[CF_QueueIdx_HIST], (CF_CListFn_t)CF_TraverseHistory, &arg);
CF_CList_Traverse(c->qs[CF_QueueIdx_HIST], (CF_CListFn_t)CF_Traverse_WriteFile, &arg);
return arg.result;
}

Expand Down
2 changes: 1 addition & 1 deletion fsw/src/cf_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,7 @@ int CF_TraverseAllTransactions_Impl(CF_CListNode_t *n, CF_TraverseAll_Arg_t *arg
* @retval CF_CLIST_CONT if everything is going well
* @retval CF_CLIST_EXIT if a write error occurred, which means traversal should stop
*/
int CF_TraverseHistory(CF_CListNode_t *n, CF_Traverse_WriteFileArg_t *context);
int CF_Traverse_WriteFile(CF_CListNode_t *n, CF_Traverse_WriteFileArg_t *context);

/************************************************************************/
/** @brief Walk over all transactions and print information from their history.
Expand Down
71 changes: 36 additions & 35 deletions unit-test/cf_utils_tests.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -532,11 +532,11 @@ void Test_CF_CList_InsertBack_Ex_Call_CF_CList_InsertBack_AndIncrement_q_size(vo

/*******************************************************************************
**
** CF_TraverseHistory tests
** CF_Traverse_WriteFile tests
**
*******************************************************************************/

void Test_CF_TraverseHistory_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM(void)
void Test_CF_Traverse_WriteFile_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM(void)
{
// /* Arrange */
// CF_History_t dummy_h;
Expand All @@ -551,14 +551,14 @@ void Test_CF_TraverseHistory_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM(void)
// dummy_h.cc = Any_condition_code_t();

// /* Act */
// CF_TraverseHistory(arg_n, arg_context);
// CF_Traverse_WriteFile(arg_n, arg_context);

// /* Assert */
UtAssert_MIR("JIRA: GSFCCFS-1733 CF_Assert - h->dir<CF_Direction_NUM");

} /* end Test_CF_TraverseHistory_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM */
} /* end Test_CF_Traverse_WriteFile_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM */

void Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT(void)
void Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT(void)
{
/* Arrange */
CF_History_t dummy_h;
Expand All @@ -580,24 +580,24 @@ void Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXI

UT_CF_ResetEventCapture(UT_KEY(CFE_EVS_SendEvent));

/* Arrange for CF_WrappedWrite in same file as CF_TraverseHistory */
/* Arrange for CF_WrappedWrite in same file as CF_Traverse_WriteFile */
dummy_len = strlen(dummy_h.fnames.src_filename) + strlen(src_colon_str);
UT_SetDeferredRetcode(UT_KEY(OS_write), FIRST_CALL, Any_int_Except(dummy_len));

/* Act */
local_result = CF_TraverseHistory(arg_n, arg_context);
local_result = CF_Traverse_WriteFile(arg_n, arg_context);

/* Assert */
UT_CF_AssertEventID(CF_EID_ERR_CMD_WHIST_WRITE);
UtAssert_True(arg_context->result == 1, "CF_TraverseHistory set context.result to %d and should be 1",
UtAssert_True(arg_context->result == 1, "CF_Traverse_WriteFile set context.result to %d and should be 1",
arg_context->result);
UtAssert_True(local_result == CF_CLIST_EXIT,
"CF_TraverseHistory returned 0x%08X and should be 0x%08X (CF_CLIST_EXIT)", local_result,
"CF_Traverse_WriteFile returned 0x%08X and should be 0x%08X (CF_CLIST_EXIT)", local_result,
CF_CLIST_EXIT);

} /* end Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT */
} /* end Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT */

void Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT(void)
void Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT(void)
{
/* Arrange */
CF_History_t dummy_h;
Expand All @@ -621,26 +621,26 @@ void Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EX

UT_CF_ResetEventCapture(UT_KEY(CFE_EVS_SendEvent));

/* Arrange for CF_WrappedWrite in same file as CF_TraverseHistory */
/* Arrange for CF_WrappedWrite in same file as CF_Traverse_WriteFile */
dummy_len_src = strlen(dummy_h.fnames.src_filename) + strlen(src_colon_str);
UT_SetDeferredRetcode(UT_KEY(OS_write), FIRST_CALL, dummy_len_src);
dummy_len_dst = strlen(dummy_h.fnames.dst_filename) + strlen(dst_colon_str);
UT_SetDeferredRetcode(UT_KEY(OS_write), NEXT_CALL, Any_int_Except(dummy_len_dst));

/* Act */
local_result = CF_TraverseHistory(arg_n, arg_context);
local_result = CF_Traverse_WriteFile(arg_n, arg_context);

/* Assert */
UT_CF_AssertEventID(CF_EID_ERR_CMD_WHIST_WRITE);
UtAssert_True(arg_context->result == 1, "CF_TraverseHistory set context.result to %d and should be 1",
UtAssert_True(arg_context->result == 1, "CF_Traverse_WriteFile set context.result to %d and should be 1",
arg_context->result);
UtAssert_True(local_result == CF_CLIST_EXIT,
"CF_TraverseHistory returned 0x%08X and should be 0x%08X (CF_CLIST_EXIT)", local_result,
"CF_Traverse_WriteFile returned 0x%08X and should be 0x%08X (CF_CLIST_EXIT)", local_result,
CF_CLIST_EXIT);

} /* end Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT */
} /* end Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT */

void Test_CF_TraverseHistory_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT(void)
void Test_CF_Traverse_WriteFile_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT(void)
{
/* Arrange */
CF_History_t dummy_h;
Expand All @@ -664,26 +664,26 @@ void Test_CF_TraverseHistory_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT(vo

UT_CF_ResetEventCapture(UT_KEY(CFE_EVS_SendEvent));

/* Arrange for CF_WrappedWrite in same file as CF_TraverseHistory */
/* Arrange for CF_WrappedWrite in same file as CF_Traverse_WriteFile */
dummy_len_src = strlen(dummy_h.fnames.src_filename) + strlen(src_colon_str);
UT_SetDeferredRetcode(UT_KEY(OS_write), FIRST_CALL, dummy_len_src);
dummy_len_dst = strlen(dummy_h.fnames.dst_filename) + strlen(dst_colon_str);
UT_SetDeferredRetcode(UT_KEY(OS_write), NEXT_CALL, dummy_len_dst);

/* Act */
local_result = CF_TraverseHistory(arg_n, arg_context);
local_result = CF_Traverse_WriteFile(arg_n, arg_context);

/* Assert */
UtAssert_STUB_COUNT(CFE_EVS_SendEvent, 0);
UtAssert_True(arg_context->result == 0, "CF_TraverseHistory context.result is %d and should be 0",
UtAssert_True(arg_context->result == 0, "CF_Traverse_WriteFile context.result is %d and should be 0",
arg_context->result);
UtAssert_True(local_result == CF_CLIST_CONT,
"CF_TraverseHistory returned 0x%08X and should be 0x%08X (CF_CLIST_CONT)", local_result,
"CF_Traverse_WriteFile returned 0x%08X and should be 0x%08X (CF_CLIST_CONT)", local_result,
CF_CLIST_CONT);

} /* end Test_CF_TraverseHistory_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT */
} /* end Test_CF_Traverse_WriteFile_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT */

/* end CF_TraverseHistory tests */
/* end CF_Traverse_WriteFile tests */

/*******************************************************************************
**
Expand All @@ -701,7 +701,7 @@ void Test_CF_TraverseTransactions_When_context_result_Is_1_Return_CLIST_EXIT(voi
CF_Traverse_WriteFileArg_t *arg_context = &dummy_context;
int local_result;

/* Arrange for CF_TraverseHistory in same file as CF_TraverseTransactions */
/* Arrange for CF_Traverse_WriteFile in same file as CF_TraverseTransactions */

dummy_t.history = &dummy_history;
dummy_t.history->src_eid = Any_uint8();
Expand Down Expand Up @@ -827,7 +827,7 @@ void Test_CF_WriteHistoryQueueDataToFile_Call_CF_CList_Traverse_AndReturn_arg_re
CF_Direction_t arg_dir = Any_direction_t();
CF_CListNode_t dummy_node;
CF_CListNode_t *expected_start = &dummy_node;
CF_CListFn_t expected_fn = (CF_CListFn_t)CF_TraverseHistory;
CF_CListFn_t expected_fn = (CF_CListFn_t)CF_Traverse_WriteFile;
int32 result;

CF_CList_Traverse_TRAV_ARG_T_context_t context_clist_traverse;
Expand Down Expand Up @@ -1465,17 +1465,18 @@ void add_cf_utils_h_tests(void)
/* end CF_CList_InsertBack_Ex tests */
}

void add_CF_TraverseHistory_tests(void)
void add_CF_Traverse_WriteFile_tests(void)
{
UtTest_Add(Test_CF_TraverseHistory_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM, cf_utils_tests_Setup,
cf_utils_tests_Teardown, "Test_CF_TraverseHistory_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM");
UtTest_Add(Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT, cf_utils_tests_Setup,
cf_utils_tests_Teardown, "Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT");
UtTest_Add(Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT, cf_utils_tests_Setup,
UtTest_Add(Test_CF_Traverse_WriteFile_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM, cf_utils_tests_Setup,
cf_utils_tests_Teardown, "Test_CF_Traverse_WriteFile_AssertsBecause_h_dir_GreaterThan_CF_DIR_NUM");
UtTest_Add(Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT, cf_utils_tests_Setup,
cf_utils_tests_Teardown,
"Test_CF_TraverseHistory_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT");
UtTest_Add(Test_CF_TraverseHistory_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT, cf_utils_tests_Setup,
cf_utils_tests_Teardown, "Test_CF_TraverseHistory_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT");
"Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsFirstCallReturn_CLIST_EXIT");
UtTest_Add(Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT, cf_utils_tests_Setup,
cf_utils_tests_Teardown,
"Test_CF_Traverse_WriteFile_When_CF_WrappedWrite_FailsSecondCallReturn_CLIST_EXIT");
UtTest_Add(Test_CF_Traverse_WriteFile_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT, cf_utils_tests_Setup,
cf_utils_tests_Teardown, "Test_CF_Traverse_WriteFile_WhenBothWrappedWritesSuccessfulReturn_CLIST_CONT");
}

void add_CF_TraverseTransactions_tests(void)
Expand Down Expand Up @@ -1591,7 +1592,7 @@ void UtTest_Setup(void)

add_cf_utils_h_tests();

add_CF_TraverseHistory_tests();
add_CF_Traverse_WriteFile_tests();

add_CF_TraverseTransactions_tests();

Expand Down

0 comments on commit 1fd43a3

Please sign in to comment.