Refactor Drv::Ip::UdpSocket to clean up state members and remove dependency on dynamic memory (#3728)

* Refactor UdpSocket to remove SocketState and the use of dynamic memory allocation.

* Replacing some ASSERTs with proper SocketIpStatus. Adding some input argument validation. Adding some better logging.

* Refactoring UdpSocket to remove redundant m_recv_port and m_recv_hostname members

* Fixing some documentation and renaming a variable to be more consistent

* Adding ADDRSTRLEN to spelling expect

* Incorporating PR fixes and adding a new UT for zero length datagram

* Replacing a strcpy with a Fw::StringUtils::string_copy

* Removing incorrect UDP statuses in empty datagram test

* Fixing bug in implementation of UdpSocket by overriding IpSocket implementation of send and recv to properly handle 0-length datagrams

* Incorporating PR fixes. Adding function argument checks for send/recv.

* Fixing build errors

* Incorporating PR comments

* Forgot to add change for IpSocket

* Fixing asserts and removing virtual from recv that we shouldn't have to override.

* Spelling

Author: vincewoo

.github/actions/spelling/expect.txt

@@ -9,6 +9,7 @@ ACTIVERATEGROUPCFG
 ACTIVERATEGROUPTESTER
 ACTIVERATEGROUPIMPLTESTER
 ACTIVETEXTLOGGERIMPL
+ADDRSTRLEN
 adminlist
 aleem
 ALLEXTERNALS

Drv/Ip/IpSocket.cpp

@@ -129,6 +129,10 @@ SocketIpStatus IpSocket::open(SocketDescriptor& socketDescriptor) {
 }
 
 SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) {
+    FW_ASSERT(socketDescriptor.fd != -1, static_cast<FwAssertArgType>(socketDescriptor.fd));
+    FW_ASSERT(data != nullptr);
+    FW_ASSERT(size > 0);
+    
     U32 total = 0;
     I32 sent  = 0;
     // Attempt to send out data and retry as necessary
@@ -161,40 +165,53 @@ SocketIpStatus IpSocket::send(const SocketDescriptor& socketDescriptor, const U8
 }
 
 SocketIpStatus IpSocket::recv(const SocketDescriptor& socketDescriptor, U8* data, U32& req_read) {
-    I32 size = 0;
-    // Try to read until we fail to receive data
-    for (U32 i = 0; (i < SOCKET_MAX_ITERATIONS) && (size <= 0); i++) {
-        errno = 0;
-        // Attempt to recv out data
-        size = this->recvProtocol(socketDescriptor, data, req_read);
+    //TODO: Uncomment FW_ASSERT for socketDescriptor.fd once we fix TcpClientTester to not pass in uninitialized socketDescriptor
+    // FW_ASSERT(socketDescriptor.fd != -1, static_cast<FwAssertArgType>(socketDescriptor.fd));
+    FW_ASSERT(data != nullptr);
 
-        // Nothing to be received
-        if ((size == -1) && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
-            req_read = 0;
-            return SOCK_NO_DATA_AVAILABLE;
-        }
+    
+    I32 bytes_received_or_status; // Stores the return value from recvProtocol
 
-        // Error is EINTR, just try again
-        if ((size == -1) && (errno == EINTR)) {
-            continue;
-        }
-        // Zero bytes read reset or bad ef means we've disconnected
-        else if (size == 0 || ((size == -1) && ((errno == ECONNRESET) || (errno == EBADF)))) {
-            req_read = static_cast<U32>(size);
-            return SOCK_DISCONNECTED;
-        }
-        // Error returned, and it wasn't an interrupt, nor a disconnect
-        else if (size == -1) {
-            req_read = static_cast<U32>(size);
-            return SOCK_READ_ERROR;  // Stop recv task on error
+    // Loop primarily for EINTR. Other conditions should lead to an earlier exit.
+    for (U32 i = 0; i < SOCKET_MAX_ITERATIONS; i++) {
+        errno = 0;
+        // Pass the current value of req_read (max buffer size) to recvProtocol.
+        // recvProtocol returns bytes read or -1 on error.
+        bytes_received_or_status = this->recvProtocol(socketDescriptor, data, req_read);
+
+        if (bytes_received_or_status > 0) {
+            // Successfully read data
+            req_read = static_cast<U32>(bytes_received_or_status);
+            return SOCK_SUCCESS;
+        } else if (bytes_received_or_status == 0) {
+            // Handle zero return based on protocol-specific behavior
+            req_read = 0;
+            return this->handleZeroReturn();
+        } else { // bytes_received_or_status == -1, an error occurred
+            if ((errno == EAGAIN) || (errno == EWOULDBLOCK)) {
+                // Non-blocking socket would block, or SO_RCVTIMEO timeout occurred.
+                req_read = 0;
+                return SOCK_NO_DATA_AVAILABLE;
+            } else if ((errno == ECONNRESET) || (errno == EBADF)) {
+                // Connection reset or bad file descriptor.
+                req_read = 0;
+                return SOCK_DISCONNECTED; // Or a more specific error like SOCK_READ_ERROR
+            } else {
+                // Other socket read error.
+                req_read = 0;
+                return SOCK_READ_ERROR;
+            }
         }
     }
-    req_read = static_cast<U32>(size);
-    // Prevent interrupted socket being viewed as success
-    if (size == -1) {
-        return SOCK_INTERRUPTED_TRY_AGAIN;
-    }
-    return SOCK_SUCCESS;
+    // If the loop completes, it means SOCKET_MAX_ITERATIONS of EINTR occurred.
+    req_read = 0;
+    return SOCK_INTERRUPTED_TRY_AGAIN;
+}
+
+SocketIpStatus IpSocket::handleZeroReturn() {
+    // For TCP (which IpSocket primarily serves as a base for, or when not overridden),
+    // a return of 0 from ::recv means the peer has performed an orderly shutdown.
+    return SOCK_DISCONNECTED;
 }
 
 }  // namespace Drv

Drv/Ip/IpSocket.hpp

@@ -114,7 +114,7 @@ class IpSocket {
      * \param size: size of data to send
      * \return status of the send, SOCK_DISCONNECTED to reopen, SOCK_SUCCESS on success, something else on error
      */
-    SocketIpStatus send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size);
+    virtual SocketIpStatus send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size);
     /**
      * \brief receive data from the IP socket from the given buffer
      *
@@ -200,13 +200,24 @@ class IpSocket {
 
     /**
      * \brief Protocol specific implementation of recv.  Called directly with error handling from recv.
-     * \param socket: socket descriptor to recv from
+     * \param socketDescriptor: socket descriptor to recv from
      * \param data: data pointer to fill
      * \param size: size of data buffer
      * \return: size of data received, or -1 on error.
      */
     virtual I32 recvProtocol(const SocketDescriptor& socketDescriptor, U8* const data, const U32 size) = 0;
 
+    /**
+     * \brief Handle zero return from recvProtocol
+     *
+     * This method is called when recvProtocol returns 0. The default implementation
+     * treats this as a disconnection (appropriate for TCP). Subclasses can override
+     * this to provide different behavior.
+     *
+     * @return SocketIpStatus Status to return from recv
+     */
+    virtual SocketIpStatus handleZeroReturn();
+
     U32 m_timeoutSeconds;
     U32 m_timeoutMicroseconds;
     U16 m_port;  //!< IP address port used