Deprecate multiple "success" code responses

[skliper]

Is your feature request related to a problem? Please describe.
API's with multiple "success" codes are frequently mishandled

Describe the solution you'd like
Single success response, unique information should be passed back in parameters

Describe alternatives you've considered
N/A

Additional context
N/A

Requester Info
Jacob Hageman - NASA/GSFC

[jphickey]

Some discussion of this topic in the context of #846 with respect to CFE_ES_ERR_SYS_LOG_TRUNCATED as (possibly) returned by CFE_ES_WriteToSysLog

My take on the topic is that all API functions should strive to meet the "ACID" test that is often employed in database software, described here:

https://en.wikipedia.org/wiki/ACID

In particular the atomicity principle (A) - A function either succeeds completely or fails completely.

Applied to something like CFE_ES_WriteToSysLog() .... the choice is simple - one must first decide whether a "job half done" in the form of a truncated message is either acceptable or it is not acceptable.

If it is decided that it is acceptable to store only a partial log message, then CFE_ES_WriteToSysLog() should return CFE_SUCCESS, not CFE_ES_ERR_SYS_LOG_TRUNCATED when this happens, because it is not an error. If the result is acceptable, it means the function was successful.

If it is decided that it is not acceptable to store only a partial log message, then CFE_ES_WriteToSysLog() should return an error code failure when this happens and write nothing at all to the log.

This way it either succeeds completely or does nothing - there should be no half-done/middle-ground/gray area where it did something but not the whole thing.

[jphickey]

I went ahead and documented all the cases of extraneous CFE "success" status/information codes. Most offenders are in TBL, a few in ES. Here is my list along with a synopsis.

CFE_ES_CDS_ALREADY_EXISTS:
- CFE_ES_RegisterCDSEx: does nothing in this case
- should be an error code, not success code.

CFE_ES_LIB_ALREADY_LOADED:
- CFE_ES_LoadLibrary: no action but does return the ID.
- Library API is (currently) non-public and nothing handles this output
- should be an error code, not success code.
- no need to return id - make a separate API to find a lib by name.

CFE_ES_ERR_SYS_LOG_TRUNCATED:
- CFE_ES_SysLogAppend_Unsync (internal) but returned through e.g. CFE_ES_WriteToSysLog
- No apps take any action on this, nor could they feasibly do anything differently.
- Not an error at all, should be CFE_SUCCESS

CFE_TBL_INFO_UPDATE_PENDING:
CFE_TBL_INFO_VALIDATION_PENDING:
CFE_TBL_INFO_DUMP_PENDING:
- All From CFE_TBL_GetStatus
- Checked for primarily by CFE_TBL_Manage, CFE_TBL_DumpToBuffer
- These are table statuses masquerading as return codes.
- To resolve, suggest a separate table state enum with these (UPDATE_PENDING, VALIDATION_PENDING, DUMP_PENDING, etc) and create a new API to query that state.

CFE_TBL_WARN_DUPLICATE:
- From CFE_TBL_Register
- TBL services appears to "re-register" and drop the old one
- Question - Why would a duplicate table registration that overwrites the old be OK where in other APIs we disallow this?
- If it is OK then response should be CFE_SUCCESS
- If it is not OK then this should be error and TBL should NOT overwrite the existing registration
- Mainly - once an app is informed of the issue, the damage is already done - the old registration is overwritten already.

CFE_TBL_INFO_UPDATED:
- From CFE_TBL_Manage (public) and CFE_TBL_GetNextNotification (internal, but status codes passed thru public APIs)
- other public APIs affected: CFE_TBL_ReleaseAddress(), CFE_TBL_GetAddress(), CFE_TBL_GetAddresses()
- For address related (CFE_TBL_ReleaseAddress(), CFE_TBL_GetAddress(), CFE_TBL_GetAddresses()) this should be SUCCESS - it got the address. This is not the right API to tell the app that the info was updated - that's irrelevant to the caller at this point, they didn't ask. (do one job, do it well - don't do other jobs)
- For CFE_TBL_Manage case the caller also didn't really ask the be informed of this. It should be SUCCESS.
- In both cases the "update" detail can be offered via new api - see CFE_TBL_INFO_UPDATE_PENDING etc.

CFE_TBL_WARN_PARTIAL_LOAD:
CFE_TBL_WARN_SHORT_FILE:
- Both of these seem like they should be errors.
- Needs requirements review....
- what is the purpose of a partial load?
- Would this cause more problems than it solves?
- Wouldn't the typical use case be to load the full table, or not at all?
- Maybe a separate API to load a table in parts/pieces that can be used by apps that actually want/need this?

CFE_TBL_INFO_NO_UPDATE_PENDING:
- From CFE_TBL_UpdateInternal, exposed via CFE_TBL_Load, CFE_TBL_Update
- In cases where it was internally invoked its probably a bug, as TBL code shouldn't invoke the internal update routine on tables not needing an update to begin with. Should be an error.
- Probably not meaningful to return to an external app/caller - makes no sense.

CFE_TBL_INFO_TABLE_LOCKED:
- From CFE_TBL_UpdateInternal
- It means it was called on a table that was locked.
- Table was not updated in this case - nothing was done AFAICT.
- So this should be an error not an info.

CFE_TBL_INFO_NO_VALIDATION_PENDING:
- From CFE_TBL_Validate
- Does an app calling need to differentiate AFTER the call to CFE_TBL_Validate, or before calling it? (in which case this is too late)
- Preemptive Info could be provided by new state query API.
- CFE_TBL_Validate should then return SUCCESS if nothing was pending.

CFE_TBL_WARN_NOT_CRITICAL:
- From CFE_TBL_Register
- Seems to happen when an app registers a critical table but the internal call to CFE_ES_RegisterCDSEx() fails.
- This should not be considered "success" .... This is a real error in that what the user asked for wasn't done.

CFE_TBL_INFO_RECOVERED_TBL:
- From CFE_TBL_Register
- Happens when an app registers a critical table and data for the table was already in CDS so it was automatically loaded
- Why was it automatically loaded in the first place? Should CFE_TBL_Register be loading tables at all?
- this isn't the patten when tables (incl critical tables) are initially loaded, where app registers and loads data into it explicitly.
- shouldn't the app dictate when (and how) table data gets recovered from CDS, just like it dictates when data gets loaded initially?
- Perhaps a separate API akin to CFE_TBL_Load (e.g. CFE_TBL_RecoverFromCDS?)
- either way should adhere to the "do one job do it well" mantra which TBL API currently violates

Edit - corrected assessment CFE_TBL_INFO_NO_UPDATE_PENDING and CFE_TBL_WARN_NOT_CRITICAL

[jphickey]

Also - noting that many offenses were in CFE_TBL_Register() and friends, I ran a cyclomatic complexity checker (cccc) on these functions.

CFE_TBL_Register() ranks in at a whopping 49 (!!!). This number is based on the number of branches and combination logic in the function. For good/maintainable code the recommendation is that a function should be no higher than 10. CFE_TBL_Load() is the next runner-up at 30.

So - it would be a wise investment to break this behemoth function up into more manageable parts.

[astrogeco]

Thanks for the thorough analysis!

[CDKnightNASA]

Excellent analysis, thanks Joe! Should we denote codes that will be deprecated or otherwise have their semantics changed? I'm thinking in cfe_error.h but possibly also in cfe_es.h/cfe_tbl.h/etc.

[skliper]

CFE_TBL_Register() ranks in at a whopping 49 (!!!). This number is based on the number of branches and combination logic in the function. For good/maintainable code the recommendation is that a function should be no higher than 10. CFE_TBL_Load() is the next runner-up at 30.

Yes, we've also been tracking cyclomatic complexity via CodeSonar reports. Not a priority for certification, but definitely on the list for a refactor. I doubt a big refactor will fit into the 7.0.0 dev cycle, so likely a target for next cycle unless it's done via contributions and even then timing is tight.