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.

Sign up for GitHub

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

Misra updates #67

Merged
merged 13 commits into from
Aug 19, 2022
Merged

Misra updates #67

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2f719e7
Added rules to the misra config file to ignore when running a coverity
Jul 11, 2022
e040a48
Updating the MISRA.md file to match the misra.config file.
Jul 11, 2022
5be4902
Removal of an enum to fix a 10.1 MISRA violation, swapping it to a
Jul 28, 2022
d46c4e9
Changing DISABLE_LOGGING to DISABLE_ASSERT to be accurate
Aug 1, 2022
cb073d7
Merge branch 'aws:main' into misraUpdates
Skptak Aug 5, 2022
cf68cef
Swapping MISRA.md format, using DNDEBUG for removal of assert when ru…
Aug 5, 2022
62d8544
Doxygen/Formatting fixes
Aug 5, 2022
3aa1a37
Doxygen changes
Aug 5, 2022
0d8d3d3
We've decided to keep the ENUM and use a switch statement to get past
Aug 17, 2022
840aa3b
Swapping to a default carrying last possible value for line coverage
Aug 17, 2022
e7a2c96
The switch statement was sending the complexity to a score of 15.
Aug 17, 2022
8ae1b1c
Updated size_table.md
Aug 17, 2022
c89b002
Turns out you only need an empty else if you have else ifs
Aug 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 9 additions & 17 deletions MISRA.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,16 @@

# MISRA Compliance

The jobs library files conform to the [MISRA C:2012](https://www.misra.org.uk)
guidelines, with some noted exceptions. Compliance is checked with Coverity static analysis.
Deviations from the MISRA standard are listed below:

### Ignored by [Coverity Configuration](tools/coverity/misra.config)
| Deviation | Category | Justification |
| :-: | :-: | :-: |
| Directive 4.9 | Advisory | Allow inclusion of function like macros. |
| Rule 3.1 | Required | Allow nested comments. C++ style `//` comments are used in example code within Doxygen documentation blocks. |
| Rule 20.12 | Required | Allow use of `assert()`, which uses a parameter in both expanded and raw forms. |
The specific deviations, suppressed inline, are listed below.

### Flagged by Coverity
| Deviation | Category | Justification |
| :-: | :-: | :-: |
| Rule 2.5 | Advisory | A macro is not used by the library; however, it exists to be used by an application. |
| Rule 8.7 | Advisory | API functions are not used by the library; however, they must be externally visible in order to be used by an application. |
Additionally, [MISRA configuration file](https://github.com/aws/Jobs-for-AWS-IoT-embedded-sdk/blob/main/tools/coverity/misra.config) contains the project wide deviations.

### Suppressed with Coverity Comments
| Deviation | Category | Justification |
| :-: | :-: | :-: |
| Rule 10.1 | Required | A variable of an enum type is used to iterate over contiguous values. |
To find the violation references in the source files run grep on the source code
with ( Assuming rule 11.4 violation; with justification in point 2 ):
```
grep 'MISRA Ref 11.1.4' . -rI
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grep is typically only found on unix operating systems by default. could we not just add the grep output in a table here? why make users run it themselves?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copied the grep from FreeRTOS+TCP MISRA changes as seen here, as I was told to make the formatting of these files/MISRA changes the same across the various repos.
Since this repo currently doesn't have any suppressed violations I see nothing wrong with removing the grep and just stating that in the file for now, I had left it there to be consistent was all.
I imagine the general idea for the inclusion of the grep instead of the table is to handle line changes when commits happen. For example corePKCS11 has upwards of 50-60 inline suppressions, so adding a table to that would require a bit of overhead to ensure it stays accurate.

```

*None.*
Skptak marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 4 additions & 4 deletions docs/doxygen/include/size_table.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@
</tr>
<tr>
<td>jobs.c</td>
<td><center>1.8K</center></td>
<td><center>1.5K</center></td>
<td><center>1.9K</center></td>
<td><center>1.6K</center></td>
</tr>
<tr>
<td><b>Total estimates</b></td>
<td><b><center>1.8K</center></b></td>
<td><b><center>1.5K</center></b></td>
<td><b><center>1.9K</center></b></td>
<td><b><center>1.6K</center></b></td>
</tr>
</table>
83 changes: 59 additions & 24 deletions source/jobs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,21 +430,35 @@ static JobsStatus_t matchIdApi( char * topic,
if( ( isNextJobId( jobId, jobIdLength ) == true ) ||
( isValidJobId( jobId, jobIdLength ) == true ) )
{
JobsTopic_t api;

/* The api variable is bounded within contiguous values of the enum type. */
/* coverity[misra_c_2012_rule_10_1_violation] */
for( api = JobsDescribeSuccess; api < JobsMaxTopic; api++ )
if( JobsSuccess == strnnEq( p, length, apiTopic[ JobsDescribeSuccess ], apiTopicLength[ JobsDescribeSuccess ] ) )
{
ret = JobsSuccess;
*outApi = JobsDescribeSuccess;
}
else if( JobsSuccess == strnnEq( p, length, apiTopic[ JobsDescribeFailed ], apiTopicLength[ JobsDescribeFailed ] ) )
{
ret = JobsSuccess;
*outApi = JobsDescribeFailed;
}
else if( JobsSuccess == strnnEq( p, length, apiTopic[ JobsUpdateSuccess ], apiTopicLength[ JobsUpdateSuccess ] ) )
{
ret = JobsSuccess;
*outApi = JobsUpdateSuccess;
}
else if( JobsSuccess == strnnEq( p, length, apiTopic[ JobsUpdateFailed ], apiTopicLength[ JobsUpdateFailed ] ) )
{
ret = JobsSuccess;
*outApi = JobsUpdateFailed;
}
else
{
ret = strnnEq( p, length, apiTopic[ api ], apiTopicLength[ api ] );
/* MISRA Empty Body */
}

if( ret == JobsSuccess )
{
*outApi = api;
*outJobId = jobId;
*outJobIdLength = jobIdLength;
break;
}
if( ret == JobsSuccess )
{
*outJobId = jobId;
*outJobIdLength = jobIdLength;
}
}

Expand All @@ -471,23 +485,44 @@ static JobsStatus_t matchApi( char * topic,
uint16_t * outJobIdLength )
{
JobsStatus_t ret = JobsNoMatch;
JobsTopic_t api;

assert( ( topic != NULL ) && ( outApi != NULL ) &&
( outJobId != NULL ) && ( outJobIdLength != NULL ) );

/* The first set of APIs do not have job IDs. */
/* The api variable is bounded within contiguous values of the enum type. */
/* coverity[misra_c_2012_rule_10_1_violation] */
for( api = JobsJobsChanged; api < JobsDescribeSuccess; api++ )
if( JobsSuccess == strnnEq( topic, topicLength, apiTopic[ JobsJobsChanged ], apiTopicLength[ JobsJobsChanged ] ) )
{
ret = strnnEq( topic, topicLength, apiTopic[ api ], apiTopicLength[ api ] );

if( ret == JobsSuccess )
{
*outApi = api;
break;
}
ret = JobsSuccess;
*outApi = JobsJobsChanged;
}
else if( JobsSuccess == strnnEq( topic, topicLength, apiTopic[ JobsNextJobChanged ], apiTopicLength[ JobsNextJobChanged ] ) )
{
ret = JobsSuccess;
*outApi = JobsNextJobChanged;
}
else if( JobsSuccess == strnnEq( topic, topicLength, apiTopic[ JobsGetPendingSuccess ], apiTopicLength[ JobsGetPendingSuccess ] ) )
{
ret = JobsSuccess;
*outApi = JobsGetPendingSuccess;
}
else if( JobsSuccess == strnnEq( topic, topicLength, apiTopic[ JobsGetPendingFailed ], apiTopicLength[ JobsGetPendingFailed ] ) )
{
ret = JobsSuccess;
*outApi = JobsGetPendingFailed;
}
else if( JobsSuccess == strnnEq( topic, topicLength, apiTopic[ JobsStartNextSuccess ], apiTopicLength[ JobsStartNextSuccess ] ) )
{
ret = JobsSuccess;
*outApi = JobsStartNextSuccess;
}
else if( JobsSuccess == strnnEq( topic, topicLength, apiTopic[ JobsStartNextFailed ], apiTopicLength[ JobsStartNextFailed ] ) )
{
ret = JobsSuccess;
*outApi = JobsStartNextFailed;
}
else
{
/* MISRA Empty Body */
}

/* The remaining APIs must have a job ID. */
Expand Down
2 changes: 2 additions & 0 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ add_library( coverity_analysis
# JOBS public include path.
target_include_directories( coverity_analysis PUBLIC ${JOBS_INCLUDE_PUBLIC_DIRS} )

# Build HTTP library target without logging
target_compile_options(coverity_analysis PUBLIC -DNDEBUG )
# ==================================== Test Configuration ========================================

# Include Unity build configuration.
Expand Down
12 changes: 8 additions & 4 deletions tools/coverity/misra.config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,19 @@
category: "Advisory",
reason: "Allow inclusion of function like macros."
},
{
deviation: "Rule 2.5",
category: "Advisory",
reason: "Allow unused macros. Library headers may define macros intended for the application's use, but not used by a specific file."
},
{
deviation: "Rule 3.1",
category: "Required",
reason: "Allow nested comments. Documentation blocks contain comments for example code."
},
{
deviation: "Rule 20.12",
category: "Required",
reason: "Allow use of assert(), which uses a parameter in both expanded and raw forms."
},
deviation: "Rule 8.7",
reason: "API functions are not used by library. They must be externally visible in order to be used by the application."
}
]
}