MQTTv5 Implementation #316
Conversation
| @@ -92,7 +92,7 @@ jobs: | |||
| uses: FreeRTOS/CI-CD-Github-Actions/complexity@main | |||
| with: | |||
| path: ./ | |||
| horrid_threshold: 12 | |||
| horrid_threshold: 17 | |||
There was a problem hiding this comment.
Why did our complexity go up so much?
There was a problem hiding this comment.
The complexity increased mainly due to functions like deserializeConnackProperties, which now has a complexity of 17. This function handles parsing of all the possible properties in a CONNACK packet (about 17 different properties). I considered splitting the logic into smaller functions, but since all of it is part of a single, cohesive task (parsing the CONNACK properties), breaking it up would have made the flow harder to follow. So we decided to increase the complexity threshold.
| ( pSubscriptionList[ iterator ].topicFilterLength == 0U ) ) | ||
| { | ||
| LogError( ( "Argument cannot be null : pTopicFilter or topicFilterLength cannot be zero. " ) ); | ||
| status = MQTTBadParameter; |
There was a problem hiding this comment.
We should validate the QoS level with the amxQoS sent by the server in the Connack as well.
| * @param[in] pContext MQTT Connection context. | ||
| * @param[in] pSubscriptionList List of MQTT subscription info. |
There was a problem hiding this comment.
The code is written in a way that this function returns true if the wildcard subscriptions are used but not supported and false if it is valid. Please update the return values documentation.
| const MQTTSubscribeInfo_t * pSubscriptionList, | ||
| const size_t iterator ) | ||
| { | ||
| MQTTStatus_t status = MQTTSuccess; |
There was a problem hiding this comment.
Please create a macro for this constant.
|
|
||
| if( bytesSentOrError != ( int32_t ) totalMessageLength ) | ||
| { | ||
| status = MQTTSendFailed; |
There was a problem hiding this comment.
We can create a local variable for pSubscriptionList[ iterator ].pTopicFilter
|
|
||
| static MQTTStatus_t validateSharedSubscriptions( const MQTTContext_t * pContext, | ||
| const MQTTSubscribeInfo_t * pSubscriptionList, | ||
| const size_t iterator ) | ||
| { | ||
| MQTTStatus_t status = MQTTSuccess; | ||
| uint16_t topicFilterLength = pSubscriptionList[ iterator ].topicFilterLength; | ||
| bool isSharedSub = ( topicFilterLength > 7U ); |
There was a problem hiding this comment.
It would be better to use the memchr function here instead of this loop.
|
|
||
| return status; | ||
| } | ||
|
|
There was a problem hiding this comment.
Please add a logerror statement here.
|
|
||
|
|
||
| if( ( pPropertyBuilder != NULL ) && ( pPropertyBuilder->pBuffer != NULL ) ) | ||
| { | ||
| propertyLength = pPropertyBuilder->currentIndex; |
There was a problem hiding this comment.
If the propbuilder is null or the buffer inside it is null then the return statement would be MQTTSuccess. But it should be MQTTBadParameter.
| { | ||
| /* MQTT DISCONNECT packets always have the same size. */ | ||
| *pPacketSize = MQTT_DISCONNECT_PACKET_SIZE; | ||
| /*Reason code.*/ |
There was a problem hiding this comment.
Please add a logerror statement here.
| status = MQTTBadParameter; | ||
| } | ||
| else if( pDisconnectInfo == NULL ) | ||
| { | ||
| status = MQTTBadParameter; | ||
| } | ||
| else if( maxPacketSize == 0U ) | ||
| { | ||
| status = MQTTBadParameter; | ||
| } | ||
| /*Packet size should not be more than the max allowed by the client.*/ | ||
| else if( ( pPacket->remainingLength + variableLengthEncodedSize( pPacket->remainingLength ) + 1U ) > maxPacketSize ) | ||
| { | ||
| status = MQTTBadResponse; |
There was a problem hiding this comment.
We should add the corresponding logerror statements at places where we assign status to MQTTBadParameter/MQTTBadResponse
| if( pPropBuffer != NULL ) | ||
| { | ||
| pPropBuffer->bufferLength = propertyLength; | ||
| pPropBuffer->pBuffer = pIndex; | ||
| } | ||
|
|
||
| /*Validate the remaining length.*/ | ||
| if( pPacket->remainingLength != ( propertyLength + variableLengthEncodedSize( propertyLength ) + 1U ) ) | ||
| { | ||
| status = MQTTBadResponse; | ||
| } |
There was a problem hiding this comment.
Shouldn't this block be within the if( status == MQTTSuccess ) block?
Upgrading MQTT Library to support v5 features
Description
Test Steps
Checklist:
Related Issue
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.