Refactor Drv::Ip::UdpSocket to clean up state members and remove dependency on dynamic memory by vincewoo · Pull Request #3728 · nasa/fprime

vincewoo · 2025-06-12T16:15:45Z


*Related Issue(s)*	#3716
*Has Unit Tests (y/n)*	y
*Documentation Included (y/n)*	n

Change Description

This change cleans up Drv::Ip::UdpSocket to remove the members m_state, m_recv_port, and m_recv_hostname in favour of two new members m_addr_send and m_addr_recv. As part of this change we are also removing the dependency on dynamic memory for storing state.

This change also improved error handling, edge case handling, and memory safety.

Rationale

Simplifies code and fixes bug of having duplicate data structures storing the same state.

Testing/Review Recommendations

Existing UTs cover the changes made except for handling 0-length datagrams. A new UT was added specifically to test the behaviour of 0-length datagrams.

Future Work

None

…y allocation.

…argument validation. Adding some better logging.

…name members

Drv/Ip/UdpSocket.cpp

LeStarch

There is a lot more here than I expected. Also, it seems that many asserts have been changed to if-checks. The general rule is:

If it is clearly a coding error, assert. (e.g. a char* was passed in as nullptr, or bind was called before open)
Use an if to check for things that could be a error with the run.

Drv/Ip/UdpSocket.cpp

LeStarch · 2025-06-12T17:56:12Z

Drv/Ip/UdpSocket.hpp

+#ifdef TGT_OS_TYPE_VXWORKS
+    #include <socket.h>
+    #include <inetLib.h>
+#elif defined TGT_OS_TYPE_LINUX || TGT_OS_TYPE_DARWIN


Perhaps we should just have the else clause and VXWORKS. Most patforms will put the headers as UNIX does.

Can we move the #include for linux/darwin into else?

Drv/Ip/UdpSocket.cpp

LeStarch

Few more cleanup items.

Drv/Ip/test/ut/TestUdp.cpp

LeStarch · 2025-06-16T21:36:26Z

Drv/Ip/UdpSocket.hpp

+#ifdef TGT_OS_TYPE_VXWORKS
+    #include <socket.h>
+    #include <inetLib.h>
+#elif defined TGT_OS_TYPE_LINUX || TGT_OS_TYPE_DARWIN


Can we move the #include for linux/darwin into else?

Drv/Ip/UdpSocket.cpp

…ementation of send and recv to properly handle 0-length datagrams

Drv/Ip/IpSocket.cpp

Drv/Ip/UdpSocket.cpp

vincewoo · 2025-06-18T16:36:42Z

@LeStarch The latest check-ins include a fix for a latent bug in the UdpSocket implementation for the handling of 0-length datagrams that you pointed out. Due to UDP supporting 0-length data and TCP not technically supporting 0-length data it required a little bit more refactoring to properly detangle the send/recv implementation in IpSocket and allow UdpSocket to override these commands.

LeStarch · 2025-06-18T17:59:15Z

Drv/Ip/test/ut/TestUdp.cpp

+    ASSERT_EQ(receiver.configureRecv("127.0.0.1", port), Drv::SOCK_SUCCESS);
+    ASSERT_EQ(receiver.open(recv_fd), Drv::SOCK_SUCCESS);
+
+    // Set a custom, longer receive timeout for this specific test to 500ms


Curious as to why....

LeStarch · 2025-06-18T18:04:18Z

Drv/Ip/test/ut/SocketTestHelper.cpp

+
+        if (status == Drv::SOCK_SUCCESS) {
+            received_size += bytes_actually_received;
+        } else if (status == Drv::SOCK_NO_DATA_AVAILABLE) {


The point of this method is to wait (even if no data is available) until such a time as the whole message received (in that the message might span multiple timeouts).

Thus I am confused why this is being commuted to a failure.

LeStarch · 2025-06-18T18:12:35Z

Drv/Ip/IpSocket.cpp

+                return SOCK_NO_DATA_AVAILABLE;
+            } else if (errno == EINTR) {
+                // Interrupted system call. Retry if not the last iteration.
+                if (i == (SOCKET_MAX_ITERATIONS - 1)) {


Why doe we need both this and the check at the end? Can't we just continue again?

LeStarch · 2025-06-18T18:16:06Z

Drv/Ip/UdpSocket.cpp

 }

+SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) {
+    // Note: socketDescriptor.fd can be -1 in some test cases


Why? How could this ever be -1?

LeStarch · 2025-06-18T18:20:51Z

Drv/Ip/UdpSocket.cpp

    return received;
 }

+SocketIpStatus UdpSocket::send(const SocketDescriptor& socketDescriptor, const U8* const data, const U32 size) {


Why do we need a special send? Can we not just update the send method on the IpSocket?

It seems odd that:

We fixed asserts in the send in IpSocket to allow size==0, but also override the method

recv is updated to respect 0 length, in Ip, but send is done in UDP.

Drv/Ip/IpSocket.cpp

-        // Nothing to be received
-        if ((size == -1) && ((errno == EAGAIN) || (errno == EWOULDBLOCK))) {
+    // Loop primarily for EINTR. Other conditions should lead to an earlier exit.
+    for (U32 i = 0; i < SOCKET_MAX_ITERATIONS; i++) {


Drv/Ip/IpSocket.cpp

+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;


Drv/Ip/UdpSocket.cpp

+    }
+
+    // For non-zero-length data, delegate to the base class implementation
+    return IpSocket::send(socketDescriptor, data, size);


Drv/Ip/UdpSocket.cpp

+    }
+
+    // For non-zero-length data, delegate to the base class implementation
+    return IpSocket::send(socketDescriptor, data, size);


…to override.

LeStarch

I need to understand this comment

//TODO: Uncomment FW_ASSERT for socketDescriptor.fd once we fix TcpClientTester to not pass in uninitialized socketDescriptor

vincewoo added 4 commits June 11, 2025 23:36

Refactor UdpSocket to remove SocketState and the use of dynamic memor…

42fbdec

…y allocation.

Replacing some ASSERTs with proper SocketIpStatus. Adding some input …

85b4568

…argument validation. Adding some better logging.

Refactoring UdpSocket to remove redundant m_recv_port and m_recv_host…

5d00932

…name members

Fixing some documentation and renaming a variable to be more consistent

006f2f6

github-advanced-security bot found potential problems Jun 12, 2025

View reviewed changes

Adding ADDRSTRLEN to spelling expect

574dba4

LeStarch self-requested a review June 12, 2025 17:44

LeStarch requested changes Jun 12, 2025

View reviewed changes

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

484c5b9

github-advanced-security bot found potential problems Jun 14, 2025

View reviewed changes

Drv/Ip/UdpSocket.cpp Fixed Show fixed Hide fixed

Drv/Ip/UdpSocket.cpp Fixed Show fixed Hide fixed

Drv/Ip/UdpSocket.cpp Fixed Show fixed Hide fixed

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

70be688

github-advanced-security bot found potential problems Jun 14, 2025

View reviewed changes

Drv/Ip/UdpSocket.cpp Fixed Show fixed Hide fixed

vincewoo requested a review from LeStarch June 14, 2025 04:15

thomas-bc changed the title ~~Refactoring Drv::Ip::UdpSocket to clean up state members and remove dependency on dynamic memory~~ Refactor Drv::Ip::UdpSocket to clean up state members and remove dependency on dynamic memory Jun 16, 2025

LeStarch requested changes Jun 16, 2025

View reviewed changes

vincewoo added 3 commits June 17, 2025 16:07

Removing incorrect UDP statuses in empty datagram test

8987394

Fixing bug in implementation of UdpSocket by overriding IpSocket impl…

401908a

…ementation of send and recv to properly handle 0-length datagrams

Merge branch 'devel' into FP-3716

2e68490

github-advanced-security bot found potential problems Jun 18, 2025

View reviewed changes

vincewoo added 2 commits June 17, 2025 21:34

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

319b38e

Fixing build errors

9ac24cd

github-advanced-security bot found potential problems Jun 18, 2025

View reviewed changes

Drv/Ip/UdpSocket.cpp Fixed Show fixed Hide fixed

vincewoo requested a review from LeStarch June 18, 2025 14:34

LeStarch reviewed Jun 18, 2025

View reviewed changes

vincewoo added 2 commits June 18, 2025 17:22

Incorporating PR comments

5a2e11d

Forgot to add change for IpSocket

9a29699

github-advanced-security bot found potential problems Jun 19, 2025

View reviewed changes

Fixing asserts and removing virtual from recv that we shouldn't have …

7bc3ab2

…to override.

Spelling

eb73a9d

vincewoo requested a review from LeStarch June 19, 2025 01:45

LeStarch reviewed Jun 26, 2025

View reviewed changes

LeStarch approved these changes Jun 26, 2025

View reviewed changes

LeStarch merged commit 3894ddc into nasa:devel Jun 26, 2025
50 checks passed

vincewoo mentioned this pull request Jun 26, 2025

Clean Up Drv::Ip Dynamic Memory #3716

Closed

vincewoo deleted the FP-3716 branch August 6, 2025 06:02

Conversation

vincewoo commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change Description

Rationale

Testing/Review Recommendations

Future Work

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeStarch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LeStarch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vincewoo commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Check warning

Check warning

Check warning

Check warning

LeStarch left a comment

vincewoo commented Jun 12, 2025 •

edited

Loading