Fix warnings in tcp/udp UT (#2744)

* Fix warnings in TCP/UDP UT * Clean unit tests * Fixes from review * Fixes from review 2 * Revert changes * Fix indentation * Fix from merge request * Fix log format * Fix indentation * add another status for no data available and allow retry in UTs --------- Co-authored-by: crsmith <celeste.r.smith@jpl.nasa.gov>
nasa · Oct 7, 2024 · 652c670 · 652c670
1 parent 65b9e4d
commit 652c670
Show file tree

Hide file tree

Showing 12 changed files with 96 additions and 37 deletions.
diff --git a/Drv/ByteStreamDriverModel/ByteStreamDriverModel.fpp b/Drv/ByteStreamDriverModel/ByteStreamDriverModel.fpp
@@ -15,7 +15,8 @@ module Drv {
     @ Status associated with the received data
     enum RecvStatus {
         RECV_OK = 0 @< Receive worked as expected
-        RECV_ERROR = 1 @< Receive error occurred retrying may succeed
+        RECV_NO_DATA = 1 @< Receive worked, but there was no data 
+        RECV_ERROR = 2 @< Receive error occurred retrying may succeed
     }
 
     @ Carries the received bytes stream driver

diff --git a/Drv/Ip/IpSocket.cpp b/Drv/Ip/IpSocket.cpp
@@ -166,8 +166,15 @@ SocketIpStatus IpSocket::recv(NATIVE_INT_TYPE fd, U8* data, U32& req_read) {
     for (U32 i = 0; (i < SOCKET_MAX_ITERATIONS) && (size <= 0); i++) {
         // Attempt to recv out data
         size = this->recvProtocol(fd, data, req_read);
+
+        // Nothing to be received
+        if ((size == -1) && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
+            req_read = 0;
+            return SOCK_NO_DATA_AVAILABLE;
+        }
+
         // Error is EINTR, just try again
-        if (size == -1 && ((errno == EINTR) || errno == EAGAIN)) {
+        if ((size == -1) && (errno == EINTR)) {
             continue;
         }
         // Zero bytes read reset or bad ef means we've disconnected

diff --git a/Drv/Ip/IpSocket.hpp b/Drv/Ip/IpSocket.hpp
@@ -36,6 +36,7 @@ enum SocketIpStatus {
     SOCK_SEND_ERROR = -13,                   //!< Failed to send after configured retries
     SOCK_NOT_STARTED = -14,                  //!< Socket has not been started
     SOCK_FAILED_TO_READ_BACK_PORT = -15,     //!< Failed to read back port from connection
+    SOCK_NO_DATA_AVAILABLE = -16             //!< No data available or read operation would block
 };
 
 /**

diff --git a/Drv/Ip/SocketComponentHelper.cpp b/Drv/Ip/SocketComponentHelper.cpp
@@ -166,9 +166,9 @@ void SocketComponentHelper::readTask(void* pointer) {
             status = self->reconnect();
             if(status != SOCK_SUCCESS) {
                 Fw::Logger::log(
-                    "[WARNING] Failed to open port with status %lu and errno %lu\n",
-                    static_cast<POINTER_CAST>(status),
-                    static_cast<POINTER_CAST>(errno));
+                    "[WARNING] Failed to open port with status %d and errno %d\n",
+                    status,
+                    errno);
                 (void) Os::Task::delay(SOCKET_RETRY_INTERVAL);
                 continue;
             }
@@ -181,10 +181,10 @@ void SocketComponentHelper::readTask(void* pointer) {
             U32 size = buffer.getSize();
             // recv blocks, so it may have been a while since its done an isOpened check
             status = self->recv(data, size);
-            if ((status != SOCK_SUCCESS) && (status != SOCK_INTERRUPTED_TRY_AGAIN)) {
-                Fw::Logger::log("[WARNING] Failed to recv from port with status %lu and errno %lu\n",
-                static_cast<POINTER_CAST>(status),
-                static_cast<POINTER_CAST>(errno));
+            if ((status != SOCK_SUCCESS) && (status != SOCK_INTERRUPTED_TRY_AGAIN && (status != SOCK_NO_DATA_AVAILABLE))) {
+                Fw::Logger::log("[WARNING] Failed to recv from port with status %d and errno %d\n",
+                    status,
+                    errno);
                 self->close();
                 buffer.setSize(0);
             } else {

diff --git a/Drv/Ip/SocketComponentHelper.hpp b/Drv/Ip/SocketComponentHelper.hpp
@@ -97,7 +97,7 @@ class SocketComponentHelper {
      * \return true if socket is open, false otherwise
      */
     bool isOpened();
-    
+
      /**
      * \brief Re-open port if it has been disconnected
      *

diff --git a/Drv/Ip/test/ut/SocketTestHelper.hpp b/Drv/Ip/test/ut/SocketTestHelper.hpp
@@ -12,7 +12,7 @@
 namespace Drv {
 namespace Test {
 
-
+static constexpr U16 MAX_ITER = 10;
 /**
  * Force a receive timeout on a socket such that it will not hang our testing despite the normal recv behavior of
  * "block forever" until it gets data.

diff --git a/Drv/TcpClient/TcpClientComponentImpl.cpp b/Drv/TcpClient/TcpClientComponentImpl.cpp
@@ -54,7 +54,16 @@ Fw::Buffer TcpClientComponentImpl::getBuffer() {
 }
 
 void TcpClientComponentImpl::sendBuffer(Fw::Buffer buffer, SocketIpStatus status) {
-    Drv::RecvStatus recvStatus = (status == SOCK_SUCCESS) ? RecvStatus::RECV_OK : RecvStatus::RECV_ERROR;
+    Drv::RecvStatus recvStatus = RecvStatus::RECV_ERROR;
+    if (status == SOCK_SUCCESS) {
+        recvStatus = RecvStatus::RECV_OK;
+    }
+    else if (status == SOCK_NO_DATA_AVAILABLE) {
+        recvStatus = RecvStatus::RECV_NO_DATA;
+    }
+    else {
+        recvStatus = RecvStatus::RECV_ERROR;
+    }
     this->recv_out(0, buffer, recvStatus);
 }
 

diff --git a/Drv/TcpClient/test/ut/TcpClientTester.cpp b/Drv/TcpClient/test/ut/TcpClientTester.cpp
@@ -65,6 +65,8 @@ void TcpClientTester ::test_with_loop(U32 iterations, bool recv_thread) {
         EXPECT_EQ(status1, Drv::SOCK_SUCCESS);
         EXPECT_EQ(status2, Drv::SOCK_SUCCESS);
 
+        status2 = Drv::SOCK_NO_DATA_AVAILABLE;
+
         // If all the opens worked, then run this
         if ((Drv::SOCK_SUCCESS == status1) && (Drv::SOCK_SUCCESS == status2) &&
             (this->component.isOpened())) {
@@ -75,7 +77,11 @@ void TcpClientTester ::test_with_loop(U32 iterations, bool recv_thread) {
             Drv::Test::fill_random_buffer(m_data_buffer);
             Drv::SendStatus status = invoke_to_send(0, m_data_buffer);
             EXPECT_EQ(status, SendStatus::SEND_OK);
-            status2 = server.recv(client_fd, buffer, size);
+            U16 counter = 0;
+            while ((status2 == Drv::SOCK_NO_DATA_AVAILABLE) and counter < Drv::Test::MAX_ITER) {
+                status2 = server.recv(client_fd, buffer, size);
+                counter++;
+            }
             EXPECT_EQ(status2, Drv::SOCK_SUCCESS);
             EXPECT_EQ(size, m_data_buffer.getSize());
             Drv::Test::validate_random_buffer(m_data_buffer, buffer);
@@ -90,7 +96,7 @@ void TcpClientTester ::test_with_loop(U32 iterations, bool recv_thread) {
             }
         }
         // Properly stop the client on the last iteration
-        if ((1 + i) == iterations && recv_thread) {
+        if (((1 + i) == iterations) && recv_thread) {
             this->component.stop();
             this->component.join();
         } else {
@@ -156,11 +162,13 @@ void TcpClientTester ::test_advanced_reconnect() {
     )
   {
     this->pushFromPortEntry_recv(recvBuffer, recvStatus);
-    // Make sure we can get to unblocking the spinner
-    EXPECT_EQ(m_data_buffer.getSize(), recvBuffer.getSize()) << "Invalid transmission size";
-    Drv::Test::validate_random_buffer(m_data_buffer, recvBuffer.getData());
-    m_data_buffer.setSize(0);
-    m_spinner = true;
+    if (recvStatus == RecvStatus::RECV_OK){
+        // Make sure we can get to unblocking the spinner
+        EXPECT_EQ(m_data_buffer.getSize(), recvBuffer.getSize()) << "Invalid transmission size";
+        Drv::Test::validate_random_buffer(m_data_buffer, recvBuffer.getData());
+        m_data_buffer.setSize(0);
+        m_spinner = true;
+    }
     delete[] recvBuffer.getData();
 }
 

diff --git a/Drv/TcpServer/TcpServerComponentImpl.cpp b/Drv/TcpServer/TcpServerComponentImpl.cpp
@@ -51,7 +51,16 @@ Fw::Buffer TcpServerComponentImpl::getBuffer() {
 }
 
 void TcpServerComponentImpl::sendBuffer(Fw::Buffer buffer, SocketIpStatus status) {
-    Drv::RecvStatus recvStatus = (status == SOCK_SUCCESS) ? RecvStatus::RECV_OK : RecvStatus::RECV_ERROR;
+    Drv::RecvStatus recvStatus = RecvStatus::RECV_ERROR;
+    if (status == SOCK_SUCCESS) {
+        recvStatus = RecvStatus::RECV_OK;
+    }
+    else if (status == SOCK_NO_DATA_AVAILABLE) {
+        recvStatus = RecvStatus::RECV_NO_DATA;
+    }
+    else {
+        recvStatus = RecvStatus::RECV_ERROR;
+    }
     this->recv_out(0, buffer, recvStatus);
 }
 

diff --git a/Drv/TcpServer/test/ut/TcpServerTester.cpp b/Drv/TcpServer/test/ut/TcpServerTester.cpp
@@ -61,6 +61,8 @@ void TcpServerTester ::test_with_loop(U32 iterations, bool recv_thread) {
         EXPECT_EQ(status1, Drv::SOCK_SUCCESS);
         EXPECT_EQ(status2, Drv::SOCK_SUCCESS);
 
+        status2 = Drv::SOCK_NO_DATA_AVAILABLE;
+
         // If all the opens worked, then run this
         if ((Drv::SOCK_SUCCESS == status1) && (Drv::SOCK_SUCCESS == status2) &&
             (this->component.isOpened())) {
@@ -72,7 +74,11 @@ void TcpServerTester ::test_with_loop(U32 iterations, bool recv_thread) {
             Drv::Test::fill_random_buffer(m_data_buffer);
             Drv::SendStatus status = invoke_to_send(0, m_data_buffer);
             EXPECT_EQ(status, SendStatus::SEND_OK);
-            status2 = client.recv(client_fd, buffer, size);
+            U16 counter = 0;
+            while ((status2 == Drv::SOCK_NO_DATA_AVAILABLE) and counter < Drv::Test::MAX_ITER) {
+                status2 = client.recv(client_fd, buffer, size);
+                counter++;
+            }
             EXPECT_EQ(status2, Drv::SOCK_SUCCESS);
             EXPECT_EQ(size, m_data_buffer.getSize());
             Drv::Test::validate_random_buffer(m_data_buffer, buffer);
@@ -89,16 +95,15 @@ void TcpServerTester ::test_with_loop(U32 iterations, bool recv_thread) {
         }
 
         // Properly stop the client on the last iteration
-        if ((1 + i) == iterations && recv_thread) {
-            this->component.shutdown();
+        if (((1 + i) == iterations) && recv_thread) {
             this->component.stop();
-            client.close(client_fd); // Client must be closed first or the server risks binding to an existing address
             this->component.close();
+            client.close(client_fd); // Client must be closed first or the server risks binding to an existing address
             this->component.shutdown();
             this->component.join();
         } else {
-            client.close(client_fd); // Client must be closed first or the server risks binding to an existing address
             this->component.close();
+            client.close(client_fd); // Client must be closed first or the server risks binding to an existing address
         }
     }
     ASSERT_from_ready_SIZE(iterations);
@@ -161,11 +166,14 @@ void TcpServerTester ::test_advanced_reconnect() {
 // ----------------------------------------------------------------------
 
 void TcpServerTester ::from_recv_handler(const NATIVE_INT_TYPE portNum, Fw::Buffer& recvBuffer, const RecvStatus& recvStatus) {
+    // this function will still receive a status of error because the recv port is always called
     this->pushFromPortEntry_recv(recvBuffer, recvStatus);
-    // Make sure we can get to unblocking the spinner
-    EXPECT_EQ(m_data_buffer.getSize(), recvBuffer.getSize()) << "Invalid transmission size";
-    Drv::Test::validate_random_buffer(m_data_buffer, recvBuffer.getData());
-    m_spinner = true;
+    if (recvStatus == RecvStatus::RECV_OK){
+        // Make sure we can get to unblocking the spinner
+        EXPECT_EQ(m_data_buffer.getSize(), recvBuffer.getSize()) << "Invalid transmission size";
+        Drv::Test::validate_random_buffer(m_data_buffer, recvBuffer.getData());
+        m_spinner = true;
+    }
     delete[] recvBuffer.getData();
 }
 

diff --git a/Drv/Udp/UdpComponentImpl.cpp b/Drv/Udp/UdpComponentImpl.cpp
@@ -62,7 +62,16 @@ Fw::Buffer UdpComponentImpl::getBuffer() {
 }
 
 void UdpComponentImpl::sendBuffer(Fw::Buffer buffer, SocketIpStatus status) {
-    Drv::RecvStatus recvStatus = (status == SOCK_SUCCESS) ? RecvStatus::RECV_OK : RecvStatus::RECV_ERROR;
+    Drv::RecvStatus recvStatus = RecvStatus::RECV_ERROR;
+    if (status == SOCK_SUCCESS) {
+        recvStatus = RecvStatus::RECV_OK;
+    }
+    else if (status == SOCK_NO_DATA_AVAILABLE) {
+        recvStatus = RecvStatus::RECV_NO_DATA;
+    }
+    else {
+        recvStatus = RecvStatus::RECV_ERROR;
+    }
     this->recv_out(0, buffer, recvStatus);
 }
 

diff --git a/Drv/Udp/test/ut/UdpTester.cpp b/Drv/Udp/test/ut/UdpTester.cpp
@@ -35,8 +35,7 @@ void UdpTester::test_with_loop(U32 iterations, bool recv_thread) {
     U16 port2 = port1;
     uint8_t attempt_to_find_available_port = std::numeric_limits<uint8_t>::max();
 
-    while ((port1 == port2) && attempt_to_find_available_port > 0)
-    {
+    while ((port1 == port2) && attempt_to_find_available_port > 0) {
         port2 = Drv::Test::get_free_port(true);
         ASSERT_NE(0, port2);
         --attempt_to_find_available_port;
@@ -83,6 +82,8 @@ void UdpTester::test_with_loop(U32 iterations, bool recv_thread) {
             << "UDP socket open error: " << strerror(errno) << std::endl
             << "Port1: " << port1 << std::endl
             << "Port2: " << port2 << std::endl;
+
+        status2 = Drv::SOCK_NO_DATA_AVAILABLE;
 
         // If all the opens worked, then run this
         if ((Drv::SOCK_SUCCESS == status1) && (Drv::SOCK_SUCCESS == status2) &&
@@ -94,7 +95,11 @@ void UdpTester::test_with_loop(U32 iterations, bool recv_thread) {
             Drv::Test::fill_random_buffer(m_data_buffer);
             Drv::SendStatus status = invoke_to_send(0, m_data_buffer);
             EXPECT_EQ(status, SendStatus::SEND_OK);
-            status2 = udp2.recv(udp2_fd, buffer, size);
+            U16 counter = 0;
+            while ((status2 == Drv::SOCK_NO_DATA_AVAILABLE) and counter < Drv::Test::MAX_ITER) {
+                status2 = udp2.recv(udp2_fd, buffer, size);
+                counter++;
+            }
             EXPECT_EQ(status2, Drv::SOCK_SUCCESS);
             EXPECT_EQ(size, m_data_buffer.getSize());
             Drv::Test::validate_random_buffer(m_data_buffer, buffer);
@@ -107,7 +112,7 @@ void UdpTester::test_with_loop(U32 iterations, bool recv_thread) {
             }
         }
         // Properly stop the client on the last iteration
-        if ((1 + i) == iterations && recv_thread) {
+        if (((1 + i) == iterations) && recv_thread) {
             this->component.stop();
             this->component.join();
         } else {
@@ -166,9 +171,11 @@ void UdpTester ::test_advanced_reconnect() {
 void UdpTester ::from_recv_handler(const NATIVE_INT_TYPE portNum, Fw::Buffer& recvBuffer, const RecvStatus& recvStatus) {
     this->pushFromPortEntry_recv(recvBuffer, recvStatus);
     // Make sure we can get to unblocking the spinner
-    EXPECT_EQ(m_data_buffer.getSize(), recvBuffer.getSize()) << "Invalid transmission size";
-    Drv::Test::validate_random_buffer(m_data_buffer, recvBuffer.getData());
-    m_spinner = true;
+    if (recvStatus == RecvStatus::RECV_OK){
+        EXPECT_EQ(m_data_buffer.getSize(), recvBuffer.getSize()) << "Invalid transmission size";
+        Drv::Test::validate_random_buffer(m_data_buffer, recvBuffer.getData());
+        m_spinner = true;
+    }
     delete[] recvBuffer.getData();
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -97,7 +97,7 @@ class SocketComponentHelper { @@
          * \return true if socket is open, false otherwise
          */
         bool isOpened();
          /**
          * \brief Re-open port if it has been disconnected
          *
@@ Expand Down @@