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

Fix MQTT MISRA violations #993

Merged
merged 7 commits into from
Jun 15, 2020
Merged

Conversation

muneebahmed10
Copy link
Contributor

@muneebahmed10 muneebahmed10 commented Jun 11, 2020

Description of changes:
Fixes most MISRA violations in MQTT. The remaining violations with our current configuration are all 8.7 (External linkage for functions used in one translation unit), which is required for API functions. Note that the changes in commit "Remove useless const" aren't due to MISRA violations, we had just been adding const because we had them in v4_beta (due to other MISRA violations relating to function pointers). However, since C is pass by value I didn't find value in keeping them.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@muneebahmed10 muneebahmed10 force-pushed the misra-mqtt branch 3 times, most recently from fe36abf to e0d8c94 Compare June 11, 2020 10:43
@muneebahmed10 muneebahmed10 marked this pull request as ready for review June 11, 2020 11:15
Comment on lines -29 to +34
#define bool signed char
#define false 0
#define true 1
#define bool int8_t
#define false ( int8_t ) 0
#define true ( int8_t ) 1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think some wanted to get rid of our #define of bool, and others wanted to keep it. I don't believe there was a clear decision on that matter, and I don't think removing it is in scope for this PR. Note that due to the changes in this PR, removal (if desired) should be straightforward, just a search and replace of bool with the desired type, false with 0, and true with 1

demos/logging-stack/logging_stack.h Outdated Show resolved Hide resolved
* from a boolean), and keeping it results in a 10.5 violation (advisory, explicit cast).
* The violation can only be resolved if the variable is of a boolean type, which is
* not present in C89. */
/* coverity[misra_c_2012_rule_10_5_violation] */
Copy link
Contributor

@abhidixi11 abhidixi11 Jun 12, 2020

Choose a reason for hiding this comment

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

We must avoid annotation, for the new library.

  1. Are we running Coverity in C99 mode ? because boolean expressions should evaluate to an integral value.
  2. What happens if bool is defined as int32_t ? Usually type of boolean expression is int ( which should be either int32_t )
  3. Other option is to change code as follows:
    if( newState == MQTTPubAckPending )
    {
    isValid = true;
    }

If there is no other way, then we should remove bool. Instead use BOOL / Bool something other than c99 bool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. I'm unsure what you mean by "C99 mode" but according to our cmake, we compile with std=c89. If we compile with C99 instead, then this isn't a violation, since the types of these values are actually booleans and not whatever we define them as for C89. This is only a violation when using C89.
  2. The same violation occurs whether we define bool as int32_t, uint32_t, uint8_t, etc. The issue is that MISRA considers the outputs of relational operators as "essentially boolean types", which is different from "essentially signed" or "essentially unsigned", and won't allow even explicit casts between them. Since there's no data type in C89 that's "essentially boolean", MISRA will complain if we store these values to a variable. So removing bool and replacing it with anything else will not help either.
  3. Yes, that is an option, however I was reluctant to do that earlier since it would greatly increase complexity of our functions due to increased nesting and lines of code, and they would have had to be split up in ways that didn't seem the most logical. However, I realized that instead of ifs, we can just use ? : instead, as that both solved the 10.5 issue and did not noticeably increase the complexity.

I reverted the annotation change and used the ternary operator instead

Copy link
Contributor

Choose a reason for hiding this comment

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

Regarding #1 and #2 I thought in C89 / ANSI mode value of boolean should be int or "essentially signed". That's why I asked if Coverity is throwing that error because it sees bool which is c99 and considers it a data type. #3 was basically the same as you change, which avoid any casts. Your change looks good, approved. ( Just make sure ternary operator is not a problem MISRA )

Copy link
Contributor

@abhidixi11 abhidixi11 left a comment

Choose a reason for hiding this comment

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

See my comment about annotations.

Copy link
Contributor

@gkwicker gkwicker left a comment

Choose a reason for hiding this comment

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

See comments

demos/logging-stack/logging_stack.h Outdated Show resolved Hide resolved
demos/logging-stack/logging_stack.h Show resolved Hide resolved
@muneebahmed10 muneebahmed10 merged commit e254aa2 into aws:development Jun 15, 2020
yourslab pushed a commit to yourslab/aws-iot-device-sdk-embedded-C that referenced this pull request Jun 17, 2020
* Fix MQTT MISRA violations

* Remove unnecessary const

* Remove unused function

* Fix string.h warning and add macro to disable logs

* Use ternary for MISRA 10.5

* Remove TODO from logging header
@muneebahmed10 muneebahmed10 deleted the misra-mqtt branch July 7, 2020 22:05
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 27, 2020
* Fix MQTT MISRA violations

* Remove unnecessary const

* Remove unused function

* Fix string.h warning and add macro to disable logs

* Use ternary for MISRA 10.5

* Remove TODO from logging header
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 28, 2020
* Fix MQTT MISRA violations

* Remove unnecessary const

* Remove unused function

* Fix string.h warning and add macro to disable logs

* Use ternary for MISRA 10.5

* Remove TODO from logging header
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Aug 31, 2020
* Fix MQTT MISRA violations

* Remove unnecessary const

* Remove unused function

* Fix string.h warning and add macro to disable logs

* Use ternary for MISRA 10.5

* Remove TODO from logging header
leegeth pushed a commit to leegeth/aws-iot-device-sdk-embedded-C that referenced this pull request Sep 1, 2020
* Fix MQTT MISRA violations

* Remove unnecessary const

* Remove unused function

* Fix string.h warning and add macro to disable logs

* Use ternary for MISRA 10.5

* Remove TODO from logging header
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants