FreeRTOS+TCP after merging with the multi branch v2 #2015
Conversation
…e windows.h (small w)
libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c
Show resolved
Hide resolved
libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c
Outdated
Show resolved
Hide resolved
libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DHCP.c
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
There are way too many rule suppressions in this code. By suppressing the warnings, we lose our visibility to them. Let's try to fix as many as we can the right way by refactoring/changing types/etc. Whatever is left, let's remove the suppressions. The point is not to get a Coverity report with zero warnings. It is to produce MISRA-compliant code. These are different objectives.
| #define ipSIZE_OF_TCP_HEADER 20u | ||
| #ifdef __COVERITY__ | ||
| /* Coverity static checks don't like inlined functions. | ||
| As it is up to t users to allow inlining, don't let |
There was a problem hiding this comment.
Changed to As it is up to the users to...
PS. when comparing inlined functions to using function-like macros, inlining clearly wins. Most compilers, when configured to optimise, don't need the __inline advice.
But: some compilers complain about static functions not being used, even when they appear in a public header file. The word __inline seems to stop those compilers from complaining.
With Coverity this trick doesn't work: it doesn't want unused functions, and it also doesn't like __inline.
|
|
||
| /* Some constants defining the sizes of several parts of a packet. | ||
| These defines come before inlucding the configuration header files. */ | ||
| #define ipSIZE_OF_ETH_HEADER 14U |
There was a problem hiding this comment.
Assuming FreeRTOS+TCP does not support 802.1Q VLAN tags here, could be worth a comment if that's the case.
There was a problem hiding this comment.
Added a comment:
+/* The size of the Ethernet header is 14, meaning that 802.1Q VLAN tags
+are not ( yet ) supported. */
#define ipSIZE_OF_ETH_HEADER 14U| @@ -114,19 +150,24 @@ typedef enum eNETWORK_EVENTS | |||
| eNetworkDown /* The network connection has been lost. */ | |||
| } eIPCallbackEvent_t; | |||
|
|
|||
| /* MISRA check: some modules refer to this typedef even tough | |||
There was a problem hiding this comment.
s/tough/though/, thank you
| eNetWorkAddressTypeIPV6, | ||
| eNetWorkAddressTypeHostName | ||
| } eNetWorkAddressType_t; | ||
| uint32_t |
There was a problem hiding this comment.
Not sure how many of these we create, but would it make sense to use a smaller type here to save a bit of memory since we only need 2 bits?
There was a problem hiding this comment.
Less than 10 instances, one for ARP, for TCP, DHCP, DNS. And in /multi, one for every end-point, used to guard the DHCP process.
The following uint8_t would not save space:
struct
{
uint8_t
bActive : 1, /* This timer is running and must be processed. */
bExpired : 1; /* Timer has expired and a task must be processed. */
TimeOut_t xTimeOut;
TickType_t ulRemainingTime;
TickType_t ulReloadTime;
};The compiler would still reserve 32 bits, because the other 3 fields need a 32-bit alignment.
| static portINLINE int32_t FreeRTOS_min_int32 (int32_t a, int32_t b) { return a <= b ? a : b; } | ||
| static portINLINE uint32_t FreeRTOS_min_uint32 (uint32_t a, uint32_t b) { return a <= b ? a : b; } | ||
| static portINLINE uint32_t FreeRTOS_round_up (uint32_t a, uint32_t d) { return d * ( ( a + d - 1u ) / d ); } | ||
| static portINLINE int32_t FreeRTOS_max_int32 (int32_t a, int32_t b) { return ( a >= b ) ? a : b; } |
There was a problem hiding this comment.
These are not networking-related, so should we define them in a networking header file? Other applications could find them useful.
There was a problem hiding this comment.
Yes you are right. There are a few more like that, like working with endianness.
The only thing that I don't know is where to put them, it must be some directory stored on Github/FreeRTOS.
libraries/freertos_plus/standard/freertos_plus_tcp/source/FreeRTOS_DNS.c
Outdated
Show resolved
Hide resolved
| struct freertos_sockaddr xAddress; | ||
| BaseType_t xReturn; | ||
|
|
||
| /* This must be the first time this function has been called. Create | ||
| the socket. */ | ||
| xSocket = FreeRTOS_socket( FREERTOS_AF_INET, FREERTOS_SOCK_DGRAM, FREERTOS_IPPROTO_UDP ); | ||
| /* coverity[misra_c_2012_rule_11_4_violation] */ | ||
| /* FREERTOS_INVALID_SOCKET is a pseudo point with a vvalue of ~0. */ |
| /* MISRA advisory rule 1.2 Relaxed in case of | ||
| configASSERT as using __FUNCTION__ makes | ||
| debugging easier */ | ||
| configASSERT( ( pcName != NULL ) ); |
There was a problem hiding this comment.
How is this rule being relaxed? There is no Coverity suppression statement here.
There was a problem hiding this comment.
The comment you see here was written earlier by Aniruddha.
I wrote this comment in coverity_misra.config:
deviation: "Rule 1.2", // Advisory, Undecidable
// Rule-1.2 Language extensions should not be used.
// Many implementations of configASSERT() use __FUNCTION__ and __LINE__
// for debugging purposes.
// configASSERT is defined outside the FreeRTOS+TCP library.
reason: "__FUNCTION__ can be useful for configASSERT()."
There are 140 calls to configASSERT() in the library. In PC-lint you can relax a rule for a single macro.
I would not like to add 140 times /* coverity[misra_c_2012_rule_1_2_violation], because ... */ along with a comment.
configASSERT() is a user-supplied macro that we can not change.
Is there a more elegant way?
| { | ||
| /* The following statement may trigger a: | ||
| warning: cast increases required alignment of target type [-Wcast-align]. | ||
| It has been confirmed though that the alignment is suitable. */ | ||
| pxResult = * ( ( NetworkBufferDescriptor_t ** ) pucBuffer ); | ||
| /* coverity[misra_c_2012_rule_11_8_violation] : he type cast of the pointer expression "A" to type "B" removes const qualifier from the pointed to type. */ | ||
| pxResult = * ( ipPOINTER_CAST( NetworkBufferDescriptor_t **, pucBuffer ) ); |
There was a problem hiding this comment.
Why are we suppressing this rule about removing the const qualifier? Is it because we can't modify this routine to return const NetworkBufferDescriptor_t * due to the "contagiousness" of const? Consider providing two versions of this function, a non-const version which has all of the code here, and a const version which just calls that one using a const * argument and returning a const * type. Code which does not modify the buffer can use the const * variant, code which does can use the other one.
There was a problem hiding this comment.
Yes, the const prefix is indeed 'contagious'!
It was not recommended by the PC-lint that I am using, but Coverity likes to turn every pointer into a const pointer where possible.
And not const char *ptr, but char const * ptr, to protect the pointee, and not the pointer.
Normally, after calling pxUDPPayloadBuffer_to_NetworkBuffer(), the resulting network buffer will be deleted, which is not a const action.
| return ipINVALID_LENGTH; | ||
| usChecksum = ipINVALID_LENGTH; | ||
| location = 1; | ||
| /* coverity[misra_c_2012_rule_15_1_violation], using goto temporarily for easier debugging.. */ |
There was a problem hiding this comment.
If this is temporary, when will it be removed?
There was a problem hiding this comment.
Non of us really likes to use goto statements, but often developers make an exception for error handling. It avoids an ever increasing nesting, cause by if statements.
Replacing the goto's with return statements is against good practice. Before this PR, returns were used.
Making a do {} while(0) loop with several breaks is also flagged by the MISRA rules. Personally, this is my favourite. It allows to have a single-point-of-return, so you can do some final actions ( like logging the type of error ).
I propose to apply a do {} while(0) loop with breaks.
B.t.w. I prepared a test for all CRC functions for IPv4 and also IPv6 that are not included yet.
| } | ||
| /*-----------------------------------------------------------*/ | ||
|
|
||
| #if 1 |
There was a problem hiding this comment.
What is the use of this #if?
* Initial commit of 85 files * Removing tools/cbmc/windows2/Windows.h (capital W), in order to create windows.h (small w) * Now adding windows2/windows.h with a small 'w' * FreeRTOS_DNS.c : always decrease 'uxSourceBytesRemaining' after using up bytes * Forgot to add these two header files * Changes after Gary's review * Removed suppressions comments for both Coverity and PC-lint in most cases Co-authored-by: Hein Tibosch <[email protected]>
FreeRTOS+TCP after merging with the multi branch v2
Description
In the last few weeks, we have been preparing a big PR #1918, with the same name as this PR.
It is now ready for reviewing and to get merged with master.
We created a new PR in order to avoid merging conflicts.
There are two groups of files in this PR:
In some aspects, the /multi version of FreeRTOS+TCP was running ahead of the /multi version.
We merged the current FreeRTOS+TCP /single with the /multi release as much as possible.
In a following stage, /multi will replace /single, and add the possibility to use multiple interfaces. And optionally, IPv6 can be enabled, running in parallel with IPv4.
The sources have passed the checks for MISRA by both Coverity and PC-lint. I places a file called freertos_plus_tcp/coverity_misra.config that was used during the checks.
Beside that, there are many cosmetic changed, e.g. use the ux prefix for all unsigned numeric variables.
Thanks to Mark Tuttle and Aniruddha Kanhere, the CBMC proofs are adapted to the new release and they are also extended.
Checklist:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.