Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cFS-Caelum Review, CFS-40: ES, Resource ID, core api and private, and build system #1283

Conversation

astrogeco
Copy link
Contributor

@astrogeco astrogeco commented Apr 7, 2021

Description

This is a "bookkeeping" Pull Request meant for the cFS-Caelum Review code inspection (full-scale code review).

This PR is solely focused on "CFS-40". For more info see this readme

The Included files are

https://github.com/astrogeco/cFE/blob/c79d66969673a37900d468203b161785bd19874f/.gitignore#L11-L49

# Build Files
!cmake/**/target_config.*
!cmake/target/CMAKELists.txt
!cmake/arch_build.cmake
!cmake/cfe_generated_file.h.in
!cmake/check_header.c.in
!cmake/generate_build_env.cmake
!cmake/generate_git_module_version.cmake
!cmake/global_functions.cmake
!cmake/mission_build.cmake
!cmake/mission_defaults.cmake
!/CMakeLists.txt

# cFE-Core
!modules/core_api/fsw/inc/cfe.h
!modules/core_api/fsw/inc/cfe_endian.h
!modules/core_api/fsw/inc/cfe_error.h
!modules/core_api/fsw/inc/cfe_version.h

# ES
!modules/core_api/fsw/inc/cfe_es*
!modules/core_private/fsw/inc/cfe_es*

!modules/es/fsw/**
!modules/es/CMakeLists.txt

# ResourceID
!modules/core_api/fsw/inc/cfe_resourceid*
!modules/core_private/fsw/inc/cfe_core_resourceid_basevalues.h

!modules/resourceid/fsw/src/*
!modules/resourceid/option_inc/*
!modules/resourceid/CMakeLists.txt
!modules/resourceid/mission_build.cmake

Objectives

  1. This review starts on 2021-04-05 and ends on 2021-04-09

  2. Dispositions of findings is on 2021-04-12

  3. Reviewers only need to review source files, header files & build files.

  4. Use .ppt, .pdf, .txt & .xlsx files for background information about the code.

  5. See the Attachments section for Peer Review Data Package.

  6. See also "The Power of 10" rules for safety-critical code. https://en.wikipedia.org/wiki/The_Power_of_10:_Rules_for_Developing_Safety-Critical_Code#:~:text=The%20Power%20of%2010%20Rules,to%20review%20or%20statically%20analyze

  7. NOTE: Don't spend too much time over coding standard violations. The Static Code Analysis tool will enforce the coding standards. This code is developed by GSFC, so GSFC coding standards will be enforced for this code base.

Notes

Note a few already existing issues (no need to individually comment on occurrences):

Doxygen event documentation doesn't match code: #508
End of function comments out of date (generalized/paraphrased version of #275)
Update code/unit tests to use CFE_Status_t: #921

If there's anything else that is observed as a repeated pattern, feel free to document as a general commen

There are several places that would trigger warnings with some common compilers/warning options. It would be nice to follow #10 rule in "The Power of 10".

Quick summary/references for currently enforced settings on the FSW
Compiler options (note -Wall and -Werror) -

add_compile_options(
-std=c99 # Target the C99 standard (without gcc extensions)
-pedantic # Issue all the warnings demanded by strict ISO C
-Wall # Warn about most questionable operations
-Wstrict-prototypes # Warn about missing prototypes
-Wwrite-strings # Warn if not treating string literals as "const"
-Wpointer-arith # Warn about suspicious pointer operations
-Wcast-align # Warn about casts that increase alignment requirements
-Werror # Treat warnings as errors (code should be clean)
)

cppcheck -

cppcheck_common_opts="--force --inline-suppr --std=c99 --language=c --enable=warning,performance,portability,style --suppress=variableScope --inconclusive"

CodeQL -

- name: Initialize CodeQL
uses: github/codeql-action/init@v1
with:
languages: c
queries: +security-extended, security-and-quality

CodeSonar - currently using default set for cFE, extending to JPL and MISRA is future work

For CodeQL and CodeSonar we don't eliminate all warnings, but we do analyze and disposition them all (plan to report dispositions as part of certification package)

This approach is compliant with the latest GSFC 582 standard (that is still going through review). Happy to discuss any additional settings that you have concerns about.

@astrogeco astrogeco force-pushed the caelum-code-review-cfs40 branch 2 times, most recently from 39cea75 to 26ae078 Compare April 8, 2021 01:17
@astrogeco astrogeco changed the title cFS-Caelum CodeReview: CFS-40 CaelumReview: CFS-40 Apr 8, 2021
@astrogeco astrogeco changed the title CaelumReview: CFS-40 Caelum Review: CFS-40 Apr 8, 2021
@astrogeco astrogeco changed the title Caelum Review: CFS-40 cFS-Caelum Review: CFS-40 Apr 8, 2021
@astrogeco astrogeco force-pushed the caelum-code-review-cfs40 branch 3 times, most recently from a842bd5 to 3f9fff4 Compare April 8, 2021 02:17
@skliper
Copy link
Contributor

skliper commented Apr 12, 2021

This round of code review has been completed, for any further issues/comments please open a new issue.

@paulober
Copy link
Contributor

Sorry for accidentally mentioning this pull request directly.

@skliper skliper added the review label Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants