From 405a99eaf10d4d186eef647a9ac4dbf4759fbaec Mon Sep 17 00:00:00 2001 From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com> Date: Thu, 29 Sep 2022 12:30:17 -0700 Subject: [PATCH 1/5] Modify check to send keep alive when no data is received --- source/core_mqtt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/core_mqtt.c b/source/core_mqtt.c index c8bd06401..177668a9b 100644 --- a/source/core_mqtt.c +++ b/source/core_mqtt.c @@ -1678,7 +1678,7 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext, totalMQTTPacketLength = incomingPacket.remainingLength + incomingPacket.headerLength; } - if( status == MQTTNoDataAvailable ) + if( recvBytes == 0 ) { if( manageKeepAlive == true ) { From dd0a95fed76d410c022e6c18a13a96e22f53039b Mon Sep 17 00:00:00 2001 From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com> Date: Thu, 29 Sep 2022 12:57:13 -0700 Subject: [PATCH 2/5] Modify control flow to process keepalive. --- source/core_mqtt.c | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/source/core_mqtt.c b/source/core_mqtt.c index 177668a9b..d7b047015 100644 --- a/source/core_mqtt.c +++ b/source/core_mqtt.c @@ -1663,7 +1663,6 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext, * the buffer. */ status = MQTTNoDataAvailable; } - /* Either something was received, or there is still data to be processed in the * buffer, or both. */ else @@ -1678,22 +1677,33 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext, totalMQTTPacketLength = incomingPacket.remainingLength + incomingPacket.headerLength; } + /* No data was received, check for keep alive timeout. */ if( recvBytes == 0 ) { if( manageKeepAlive == true ) { + /* Keep the copy of the status to be reset later. */ + MQTTStatus_t statusCopy = status; + /* Assign status so an error can be bubbled up to application, * but reset it on success. */ status = handleKeepAlive( pContext ); - } - if( status == MQTTSuccess ) - { - /* Reset the status to indicate that nothing was read - * from the transport interface. */ - status = MQTTNoDataAvailable; + if( status == MQTTSuccess ) + { + /* Reset the status. */ + status = statusCopy; + } } } + + /* Check whether there is data available before processing the packet further. */ + if( ( status == MQTTNeedMoreBytes ) || ( status == MQTTNoDataAvailable ) ) + { + /* Do nothing as there is nothing to be processed right now. The proper + * error code will be bubbled up to the user. */ + } + /* Any other error code. */ else if( status != MQTTSuccess ) { LogError( ( "Receiving incoming packet length failed. Status=%s", @@ -1702,7 +1712,8 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext, /* If the MQTT Packet size is bigger than the buffer itself. */ else if( totalMQTTPacketLength > pContext->networkBuffer.size ) { - /* Discard the packet from the buffer and from the socket buffer. */ + /* Discard the packet from the receive buffer and drain the pending + * data from the socket buffer. */ status = discardStoredPacket( pContext, &incomingPacket ); } From 6391c34e288dee033a38db1d022f30aa2da0c8ec Mon Sep 17 00:00:00 2001 From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com> Date: Thu, 29 Sep 2022 21:12:47 +0000 Subject: [PATCH 3/5] Fix formatting and increase coverage --- source/core_mqtt.c | 1 + test/unit-test/core_mqtt_utest.c | 118 +++++++++++++++++++++++++++---- 2 files changed, 107 insertions(+), 12 deletions(-) diff --git a/source/core_mqtt.c b/source/core_mqtt.c index d7b047015..421c8d332 100644 --- a/source/core_mqtt.c +++ b/source/core_mqtt.c @@ -1663,6 +1663,7 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext, * the buffer. */ status = MQTTNoDataAvailable; } + /* Either something was received, or there is still data to be processed in the * buffer, or both. */ else diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c index d274e5aeb..7fcab5f42 100644 --- a/test/unit-test/core_mqtt_utest.c +++ b/test/unit-test/core_mqtt_utest.c @@ -3766,6 +3766,7 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths1( void ) setupNetworkBuffer( &networkBuffer ); setupTransportInterface( &transport ); transport.writev = transportWritevSuccess; + transport.recv = transportRecvNoData; globalEntryTime = MQTT_PINGRESP_TIMEOUT_MS + 1; @@ -3778,21 +3779,11 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths1( void ) context.lastPacketRxTime = 0; context.pingReqSendTimeMs = 0; context.waitingForPingResp = true; + /* Set expected return values in the loop. */ resetProcessLoopParams( &expectParams ); expectParams.processLoopStatus = MQTTKeepAliveTimeout; - MQTTPacketInfo_t incomingPacket = { 0 }; - /* Modify incoming packet depending on type to be tested. */ - currentPacketType = MQTT_PACKET_TYPE_PINGRESP; - incomingPacket.type = currentPacketType; - incomingPacket.remainingLength = MQTT_SAMPLE_REMAINING_LENGTH; - incomingPacket.headerLength = MQTT_SAMPLE_REMAINING_LENGTH; - - MQTT_ProcessIncomingPacketTypeAndLength_ExpectAnyArgsAndReturn( MQTTNoDataAvailable ); - MQTT_ProcessIncomingPacketTypeAndLength_ReturnThruPtr_pIncomingPacket( &incomingPacket ); - - mqttStatus = MQTT_ProcessLoop( &context ); TEST_ASSERT_EQUAL( MQTTKeepAliveTimeout, mqttStatus ); } @@ -3859,6 +3850,7 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths3( void ) setupTransportInterface( &transport ); setupNetworkBuffer( &networkBuffer ); + transport.recv = transportRecvNoData; modifyIncomingPacketStatus = MQTTNoDataAvailable; globalEntryTime = MQTT_PINGRESP_TIMEOUT_MS + 1; @@ -3876,6 +3868,52 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths3( void ) /* Set expected return values in the loop. */ resetProcessLoopParams( &expectParams ); + MQTT_GetPingreqPacketSize_ExpectAnyArgsAndReturn( MQTTSuccess ); + MQTT_SerializePingreq_ExpectAnyArgsAndReturn( MQTTSuccess ); + + mqttStatus = MQTT_ProcessLoop( &context ); + TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus ); +} + +/** + * @brief This test case covers all calls to the private method, + * handleKeepAlive(...), + * that result in the process loop returning an error. + */ +void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths4( void ) +{ + MQTTStatus_t mqttStatus; + MQTTContext_t context = { 0 }; + TransportInterface_t transport = { 0 }; + MQTTFixedBuffer_t networkBuffer = { 0 }; + ProcessLoopReturns_t expectParams = { 0 }; + + setupTransportInterface( &transport ); + transport.recv = transportRecvNoData; + + setupNetworkBuffer( &networkBuffer ); + + modifyIncomingPacketStatus = MQTTNoDataAvailable; + globalEntryTime = MQTT_PINGRESP_TIMEOUT_MS + 1; + + /* Coverage for the branch path where PINGRESP timeout interval has expired. */ + mqttStatus = MQTT_Init( &context, &transport, getTime, eventCallback, &networkBuffer ); + TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus ); + + globalEntryTime = PACKET_RX_TIMEOUT_MS + 1; + context.keepAliveIntervalSec = ( PACKET_TX_TIMEOUT_MS / 1000 ) + 1U; + context.lastPacketTxTime = 0; + context.lastPacketRxTime = 0; + context.pingReqSendTimeMs = 0; + context.waitingForPingResp = false; + + /* Set the index to non-zero value to show that there is some data in the buffer + * to be processed. */ + context.index = 12; + + /* Set expected return values in the loop. */ + resetProcessLoopParams( &expectParams ); + MQTTPacketInfo_t incomingPacket = { 0 }; /* Modify incoming packet depending on type to be tested. */ currentPacketType = MQTT_PACKET_TYPE_PINGRESP; @@ -3883,14 +3921,70 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths3( void ) incomingPacket.remainingLength = MQTT_SAMPLE_REMAINING_LENGTH; incomingPacket.headerLength = MQTT_SAMPLE_REMAINING_LENGTH; - MQTT_ProcessIncomingPacketTypeAndLength_ExpectAnyArgsAndReturn( MQTTNoDataAvailable ); + MQTT_ProcessIncomingPacketTypeAndLength_ExpectAnyArgsAndReturn( MQTTNeedMoreBytes ); MQTT_ProcessIncomingPacketTypeAndLength_ReturnThruPtr_pIncomingPacket( &incomingPacket ); MQTT_GetPingreqPacketSize_ExpectAnyArgsAndReturn( MQTTSuccess ); MQTT_SerializePingreq_ExpectAnyArgsAndReturn( MQTTSuccess ); mqttStatus = MQTT_ProcessLoop( &context ); + TEST_ASSERT_EQUAL( MQTTNeedMoreBytes, mqttStatus ); +} + +/** + * @brief This test case covers all calls to the private method, + * handleKeepAlive(...), + * that result in the process loop returning an error. + */ +void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths5( void ) +{ + MQTTStatus_t mqttStatus; + MQTTContext_t context = { 0 }; + TransportInterface_t transport = { 0 }; + MQTTFixedBuffer_t networkBuffer = { 0 }; + ProcessLoopReturns_t expectParams = { 0 }; + + setupTransportInterface( &transport ); + transport.recv = transportRecvNoData; + + setupNetworkBuffer( &networkBuffer ); + + modifyIncomingPacketStatus = MQTTNoDataAvailable; + globalEntryTime = MQTT_PINGRESP_TIMEOUT_MS + 1; + + /* Coverage for the branch path where PINGRESP timeout interval has expired. */ + mqttStatus = MQTT_Init( &context, &transport, getTime, eventCallback, &networkBuffer ); TEST_ASSERT_EQUAL( MQTTSuccess, mqttStatus ); + + globalEntryTime = PACKET_RX_TIMEOUT_MS - 1U; + context.keepAliveIntervalSec = ( PACKET_TX_TIMEOUT_MS / 1000 ) + 1U; + context.lastPacketTxTime = 0; + context.lastPacketRxTime = 0; + context.pingReqSendTimeMs = 0; + context.waitingForPingResp = false; + + /* Set the index to non-zero value to show that there is some data in the buffer + * to be processed. */ + context.index = 12; + + /* Set expected return values in the loop. */ + resetProcessLoopParams( &expectParams ); + + MQTTPacketInfo_t incomingPacket = { 0 }; + /* Modify incoming packet depending on type to be tested. */ + currentPacketType = MQTT_PACKET_TYPE_PINGRESP; + incomingPacket.type = currentPacketType; + incomingPacket.remainingLength = MQTT_SAMPLE_REMAINING_LENGTH; + incomingPacket.headerLength = MQTT_SAMPLE_REMAINING_LENGTH; + + MQTT_ProcessIncomingPacketTypeAndLength_ExpectAnyArgsAndReturn( MQTTNeedMoreBytes ); + MQTT_ProcessIncomingPacketTypeAndLength_ReturnThruPtr_pIncomingPacket( &incomingPacket ); + + MQTT_GetPingreqPacketSize_ExpectAnyArgsAndReturn( MQTTSuccess ); + MQTT_SerializePingreq_ExpectAnyArgsAndReturn( MQTTSuccess ); + + mqttStatus = MQTT_ProcessLoop( &context ); + TEST_ASSERT_EQUAL( MQTTNeedMoreBytes, mqttStatus ); } /** From 31e363208936d9b5442b5ae37d9c660645822ab7 Mon Sep 17 00:00:00 2001 From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com> Date: Thu, 29 Sep 2022 22:15:53 +0000 Subject: [PATCH 4/5] Remove unused mocked functions --- test/unit-test/core_mqtt_utest.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/unit-test/core_mqtt_utest.c b/test/unit-test/core_mqtt_utest.c index 7fcab5f42..21fb5d235 100644 --- a/test/unit-test/core_mqtt_utest.c +++ b/test/unit-test/core_mqtt_utest.c @@ -3980,9 +3980,6 @@ void test_MQTT_ProcessLoop_handleKeepAlive_Error_Paths5( void ) MQTT_ProcessIncomingPacketTypeAndLength_ExpectAnyArgsAndReturn( MQTTNeedMoreBytes ); MQTT_ProcessIncomingPacketTypeAndLength_ReturnThruPtr_pIncomingPacket( &incomingPacket ); - MQTT_GetPingreqPacketSize_ExpectAnyArgsAndReturn( MQTTSuccess ); - MQTT_SerializePingreq_ExpectAnyArgsAndReturn( MQTTSuccess ); - mqttStatus = MQTT_ProcessLoop( &context ); TEST_ASSERT_EQUAL( MQTTNeedMoreBytes, mqttStatus ); } From d709fc121f8dc51727f30145b1c384245f90caec Mon Sep 17 00:00:00 2001 From: Aniruddha Kanhere <60444055+AniruddhaKanhere@users.noreply.github.com> Date: Thu, 29 Sep 2022 22:53:12 +0000 Subject: [PATCH 5/5] Update log messages --- source/core_mqtt.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source/core_mqtt.c b/source/core_mqtt.c index 421c8d332..e1e9c5f89 100644 --- a/source/core_mqtt.c +++ b/source/core_mqtt.c @@ -1695,6 +1695,11 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext, /* Reset the status. */ status = statusCopy; } + else + { + LogError( ( "Handling of keep alive failed. Status=%s", + MQTT_Status_strerror( status ) ) ); + } } } @@ -1707,7 +1712,7 @@ static MQTTStatus_t receiveSingleIteration( MQTTContext_t * pContext, /* Any other error code. */ else if( status != MQTTSuccess ) { - LogError( ( "Receiving incoming packet length failed. Status=%s", + LogError( ( "Call to receiveSingleIteration failed. Status=%s", MQTT_Status_strerror( status ) ) ); } /* If the MQTT Packet size is bigger than the buffer itself. */