Fix #225, refactor goto statements by havencarlson · Pull Request #269 · nasa/CF

havencarlson · 2022-06-22T21:36:53Z

Checklist (Please check before submitting)

I reviewed the Contributing Guide.
I signed and emailed the appropriate Contributor License Agreement to [email protected] and copied [email protected].

Describe the contribution
Fix #225, replaced all instances of goto

Testing performed
Ran unit tests

Expected behavior changes
No impact to behavior

System(s) tested on
Ubuntu 18.04

Contributor Info - All information REQUIRED for consideration of pull request
Haven Carlson - NASA

fsw/src/cf_cfdp.c

@@ -654,18 +651,15 @@ int CF_CFDP_RecvPh(uint8 chan_num, CF_Logical_PduBuffer_t *ph)
    {


fsw/src/cf_cfdp.c

skliper

Preference is for functions to have one exit point if reasonably possible. We don't currently enforce it as a hard rule but should be the goal.

github-advanced-security

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 17 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 50 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 64 potential problems in the proposed changes. Check the Files changed tab for more details.

fsw/src/cf_cfdp_s.c

fsw/src/cf_cfdp_sbintf.c

        /* no more messages this wakeup allowed */
-        c->cur = t; /* remember where we were for next time */
-        goto error_out;
+        c->cur  = t; /* remember where we were for next time */


github-advanced-security

Found 61 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 61 potential problems in the proposed changes. Check the Files changed tab for more details.

havencarlson · 2022-07-14T20:22:43Z

I believe I got all functions. The only function I wasn't sure what to do with was CF_CFDP_S_Tick() in cf_cfdp_s.c

skliper

Great progress! See suggestions (happy to chat if it helps.)

skliper · 2022-07-14T21:52:29Z

fsw/src/cf_cfdp_s.c

-     */
-    fd->data_len = actual_bytes;
-    fd->data_ptr = data_ptr;
+        int status;


Declare at the top of the function... might also need to initialize it just to squash warnings

skliper · 2022-07-14T21:58:30Z

fsw/src/cf_cfdp_s.c

            ++CF_AppData.hk.channel_hk[t->chan_num].counters.fault.file_open;
-            goto err_out;
+            t->history->cc = CF_CFDP_ConditionCode_FILESTORE_REJECTION;
+            CF_CFDP_S_Reset(t);


Does this work because the rest of the calls fail on a transaction that was reset? Probably more clear if you set a flag again and only did each of the following calls based on success. Could also just do the reset and filestore rejection once at the end if it wasn't success.

skliper · 2022-07-14T22:03:37Z

fsw/src/cf_cfdp_s.c

                            /* no reason to reset this timer, as it isn't used again */
                            CF_CFDP_S_Reset(t);
-                            goto err_out; /* must exit after reset */
+                            return; /* must exit after reset */


I need to think more on this one... but I think we can simplify. If the transaction is reset will it skip the if on 792 anyways? Seem like you just need to also skip the finack send if the SendEof return is CF_SendRet_NO_MSG...

skliper · 2022-07-14T22:06:04Z

fsw/src/cf_clist.c

            if (fn(n, context))
            {
-                goto err_out;
+                return;


Could you just break?

skliper · 2022-07-14T22:06:28Z

fsw/src/cf_clist.c

                if (fn(n, context))
                {
-                    goto err_out;
+                    return;


just break if you can

skliper · 2022-07-14T22:12:19Z

fsw/src/cf_app.c

-    status = CFE_TBL_Register(&CF_AppData.config_handle, CF_CONFIG_TABLE_NAME, sizeof(CF_ConfigTable_t),
-                              CFE_TBL_OPT_SNGL_BUFFER | CFE_TBL_OPT_LOAD_DUMP, CF_ValidateConfigTable);
-    if (status != CFE_SUCCESS)
+    if ((status = CFE_TBL_Register(&CF_AppData.config_handle, CF_CONFIG_TABLE_NAME, sizeof(CF_ConfigTable_t),


Preference is to avoid side affects in conditionals... get the status first, then the decision is just status != CFE_SUCCESS (same for similar patterns below)

skliper · 2022-07-14T22:14:12Z

fsw/src/cf_app.c

 int32 CF_Init(void)
 {
    int32                            status       = CFE_SUCCESS;
+    bool                             success      = true;


can you just use status == CFE_SUCCESS for these? Might be able to replace at least the first decision w/ just an else (maybe more?)

skliper · 2022-07-14T22:17:15Z

fsw/src/cf_cfdp.c

                    break; /* triggers tick type reset below */
                else
-                    goto early_exit;
+                    return;


maybe set an "early exit" then only reset the tick type below if not an early exit?

When I tried to update this, it seemed to break a unit test and I'm not sure why.

Did you try on line 1240 just doing a break; (since below it just breaks or returns), and then move the conditional on 1256 (might need to adjust the comment if it makes sense) to wrap the logic on 1266? Basically you just want to reset the tick type if c->tick_type == CF_TickType_TXW_NAK.

skliper · 2022-07-14T22:18:44Z

fsw/src/cf_cfdp_r.c

    if (t->history->cc != CF_CFDP_ConditionCode_NO_ERROR)
    {
-        goto err_out; /* nothing to do here if error cc is set */
+        return; /* nothing to do here if error cc is set */


Could just switch to an == and wrap the entire block blow

skliper · 2022-07-14T22:23:36Z

fsw/src/cf_cfdp_r.c

                        ++CF_AppData.hk.channel_hk[t->chan_num].counters.fault.ack_limit;
                        CF_CFDP_R2_Reset(t);
-                        goto err_out; /* must return after reset */
+                        return; /* must return after reset */


I think you just need to skip the CF_CFDP_ArmAckTimer... and could let it flow to the end

github-advanced-security

Found 69 potential problems in the proposed changes. Check the Files changed tab for more details.

fsw/src/cf_cfdp_s.c

fsw/src/cf_app.c

github-advanced-security

Found 69 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 68 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 68 potential problems in the proposed changes. Check the Files changed tab for more details.

fsw/src/cf_app.c

skliper · 2022-07-20T12:50:05Z

fsw/src/cf_cfdp.c

                    break; /* triggers tick type reset below */
                else
-                    goto early_exit;
+                    return;


Did you try on line 1240 just doing a break; (since below it just breaks or returns), and then move the conditional on 1256 (might need to adjust the comment if it makes sense) to wrap the logic on 1266? Basically you just want to reset the tick type if c->tick_type == CF_TickType_TXW_NAK.

fsw/src/cf_cfdp_s.c

github-advanced-security

Found 68 potential problems in the proposed changes. Check the Files changed tab for more details.

github-advanced-security

Found 68 potential problems in the proposed changes. Check the Files changed tab for more details.

dzbaker · 2022-07-20T16:09:21Z

CCB 20 Jul 2022: Approved pending final review by @semaldona.

havencarlson added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jun 22, 2022

github-advanced-security bot found potential problems Jun 22, 2022

View reviewed changes

skliper suggested changes Jun 23, 2022

View reviewed changes

havencarlson force-pushed the fix#225-goto branch 2 times, most recently from 453ae2d to 58e591e Compare June 24, 2022 20:42

github-advanced-security bot found potential problems Jun 24, 2022

View reviewed changes

havencarlson force-pushed the fix#225-goto branch from 58e591e to 07f9228 Compare June 24, 2022 20:45

github-advanced-security bot found potential problems Jun 24, 2022

View reviewed changes

havencarlson requested a review from skliper June 27, 2022 14:14

havencarlson removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Jun 29, 2022

havencarlson force-pushed the fix#225-goto branch from 07f9228 to 8dac0a2 Compare July 14, 2022 19:04

github-advanced-security bot found potential problems Jul 14, 2022

View reviewed changes

havencarlson force-pushed the fix#225-goto branch from 8dac0a2 to 0012263 Compare July 14, 2022 19:56

github-advanced-security bot found potential problems Jul 14, 2022

View reviewed changes

fsw/src/cf_cfdp_s.c Fixed Show fixed Hide fixed

havencarlson force-pushed the fix#225-goto branch from 0012263 to 88d8764 Compare July 14, 2022 20:09

github-advanced-security bot found potential problems Jul 14, 2022

View reviewed changes

havencarlson force-pushed the fix#225-goto branch from 88d8764 to d628409 Compare July 14, 2022 20:15

github-advanced-security bot found potential problems Jul 14, 2022

View reviewed changes

skliper suggested changes Jul 14, 2022

View reviewed changes

havencarlson force-pushed the fix#225-goto branch from d628409 to 9c47813 Compare July 20, 2022 03:32