refactor: S13.7 extract Datagram abstraction from UdpSender#139
Merged
Conversation
Extract UDP socket operations into a SolidSyslogDatagram vtable abstraction, following the same pattern as S13.6 Resolver. PosixDatagram provides socket/sendto/close for POSIX platforms. UdpSender wiring will follow in a subsequent commit. Open/Close return void (no error handling today — Epic #31). SendTo returns bool, preserving the existing sendto >= 0 semantics.
Add datagram to UdpSenderConfig. Remove internal socket/sendto/close and the fd field — UdpSender now calls through the injected Datagram. Sender retains protocol logic and owns the resolved address; the Datagram owns the socket resource lifecycle. All existing UdpSender tests pass unchanged — SocketFake intercepts at the libc level, so the observable behaviour through PosixDatagram is identical. Examples and ExampleServiceThreadTest will be updated in the next commit.
Both examples and the ExampleServiceThreadTest now create a PosixDatagram and pass it in UdpSenderConfig. TcpSender is unchanged (S13.8 will extract the Stream abstraction).
Contributor
📝 WalkthroughWalkthroughThe PR extracts UDP socket operations from Changes
Sequence DiagramsequenceDiagram
participant UdpSender
participant SolidSyslogDatagram as Datagram<br/>(Dispatch)
participant PosixDatagram as PosixDatagram<br/>(Implementation)
participant Socket as POSIX Socket
Note over UdpSender,Socket: OLD: Direct socket management
UdpSender->>Socket: socket(AF_INET, SOCK_DGRAM)
Socket-->>UdpSender: fd
Note over UdpSender,Socket: OLD: Send operation
UdpSender->>Socket: sendto(fd, buffer, addr)
Socket-->>UdpSender: bytes sent (>= 0)
UdpSender->>Socket: close(fd)
Note over UdpSender,Socket: NEW: Abstracted socket management
UdpSender->>SolidSyslogDatagram: SolidSyslogDatagram_Open(datagram)
SolidSyslogDatagram->>PosixDatagram: datagram->Open(self)
PosixDatagram->>Socket: socket(AF_INET, SOCK_DGRAM)
Socket-->>PosixDatagram: fd
Note over UdpSender,Socket: NEW: Send operation
UdpSender->>SolidSyslogDatagram: SolidSyslogDatagram_SendTo(datagram, buffer, addr)
SolidSyslogDatagram->>PosixDatagram: datagram->SendTo(self, buffer, addr)
PosixDatagram->>Socket: sendto(fd, buffer, addr)
Socket-->>PosixDatagram: bytes sent
PosixDatagram-->>SolidSyslogDatagram: bool (success)
SolidSyslogDatagram-->>UdpSender: bool (success)
UdpSender->>SolidSyslogDatagram: SolidSyslogDatagram_Close(datagram)
SolidSyslogDatagram->>PosixDatagram: datagram->Close(self)
PosixDatagram->>Socket: close(fd)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 495 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
Source/SolidSyslogPosixDatagram.c (1)
29-34: MakeSolidSyslogPosixDatagram_Destroydefensively close any open socket.Right now Destroy only nulls function pointers. If
fdis still open, it can leak a descriptor.Suggested patch
void SolidSyslogPosixDatagram_Destroy(void) { + Close(&instance.base); instance.base.Open = NULL; instance.base.SendTo = NULL; instance.base.Close = NULL; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Source/SolidSyslogPosixDatagram.c` around lines 29 - 34, SolidSyslogPosixDatagram_Destroy currently only nulls function pointers and can leak the open socket; update it to check the instance.fd (or equivalent file descriptor field on the instance object) and if it is >= 0 call close(fd) (include the proper header if needed), then set instance.fd = -1 and finally null out instance.base.Open/SendTo/Close as before to ensure the descriptor is closed defensively; reference the SolidSyslogPosixDatagram_Destroy function and the instance.fd field when making the change.Tests/SolidSyslogPosixDatagramTest.cpp (1)
63-67: Optional: extract repeatedOpen + SendToarrange/act into a helper.This would reduce duplication and make intent in each test even clearer.
♻️ Suggested test refactor
TEST_GROUP(SolidSyslogPosixDatagram) { @@ + void OpenAndSendDefault() const + { + SolidSyslogDatagram_Open(datagram); + SolidSyslogDatagram_SendTo(datagram, TEST_MESSAGE, TEST_MESSAGE_LEN, &addr); + } @@ TEST(SolidSyslogPosixDatagram, SendToCallsSendtoOnce) { - SolidSyslogDatagram_Open(datagram); - SolidSyslogDatagram_SendTo(datagram, TEST_MESSAGE, TEST_MESSAGE_LEN, &addr); + OpenAndSendDefault(); LONGS_EQUAL(1, SocketFake_SendtoCallCount()); }Also applies to: 72-74, 79-81, 86-88, 93-95, 100-102, 107-109, 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/SolidSyslogPosixDatagramTest.cpp` around lines 63 - 67, Extract the repeated "open then send" setup into a small test helper to reduce duplication: create a helper function (e.g., SendOpenAndSendTo or Arrange_OpenAndSendTo) that calls SolidSyslogDatagram_Open(datagram) and SolidSyslogDatagram_SendTo(datagram, TEST_MESSAGE, TEST_MESSAGE_LEN, &addr) and use that helper in tests currently calling SolidSyslogDatagram_Open + SolidSyslogDatagram_SendTo (tests referencing SendToCallsSendtoOnce and the similar cases at lines noted) so each test becomes a single clear arrange/act call followed by assertions like LONGS_EQUAL(1, SocketFake_SendtoCallCount()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Source/SolidSyslogPosixDatagram.c`:
- Around line 29-34: SolidSyslogPosixDatagram_Destroy currently only nulls
function pointers and can leak the open socket; update it to check the
instance.fd (or equivalent file descriptor field on the instance object) and if
it is >= 0 call close(fd) (include the proper header if needed), then set
instance.fd = -1 and finally null out instance.base.Open/SendTo/Close as before
to ensure the descriptor is closed defensively; reference the
SolidSyslogPosixDatagram_Destroy function and the instance.fd field when making
the change.
In `@Tests/SolidSyslogPosixDatagramTest.cpp`:
- Around line 63-67: Extract the repeated "open then send" setup into a small
test helper to reduce duplication: create a helper function (e.g.,
SendOpenAndSendTo or Arrange_OpenAndSendTo) that calls
SolidSyslogDatagram_Open(datagram) and SolidSyslogDatagram_SendTo(datagram,
TEST_MESSAGE, TEST_MESSAGE_LEN, &addr) and use that helper in tests currently
calling SolidSyslogDatagram_Open + SolidSyslogDatagram_SendTo (tests referencing
SendToCallsSendtoOnce and the similar cases at lines noted) so each test becomes
a single clear arrange/act call followed by assertions like LONGS_EQUAL(1,
SocketFake_SendtoCallCount()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c15be25f-bec9-46ec-8a14-19070eb00b66
📒 Files selected for processing (14)
Example/SingleTask/SolidSyslogExample.cExample/Threaded/main.cInterface/SolidSyslogDatagram.hInterface/SolidSyslogDatagramDefinition.hInterface/SolidSyslogPosixDatagram.hInterface/SolidSyslogUdpSender.hSource/CMakeLists.txtSource/SolidSyslogDatagram.cSource/SolidSyslogPosixDatagram.cSource/SolidSyslogUdpSender.cTests/CMakeLists.txtTests/Example/ExampleServiceThreadTest.cppTests/SolidSyslogPosixDatagramTest.cppTests/SolidSyslogUdpSenderTest.cpp
Owner
Author
|
Thanks for the review. Both nitpicks are deferred — rationale below.
|
This was referenced Apr 17, 2026
This was referenced Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Closes #133. Extract UDP socket operations from
UdpSenderinto aSolidSyslogDatagramvtable abstraction, withPosixDatagramas the first implementation. Preparatory refactoring for S13.4 (Winsock UDP support) — once Datagram is extracted, adding Windows support is just a newWinsockDatagramimplementation.Parent epic: #40. Depends on S13.6 (#132, already merged).
Change Description
Follows the same vtable/dispatch pattern as Sender, Buffer, Store, Resolver:
SolidSyslogDatagramDefinition.h— vtable struct withOpen,SendTo,CloseSolidSyslogDatagram.h/.c— forward declaration and dispatch functionsSolidSyslogPosixDatagram.h/.c— POSIX implementation usingsocket/sendto/closeUdpSenderrefactored to accept aSolidSyslogDatagram*in its config; no longer touches sockets directly.Design decisions (following S13.6 pattern):
OpenandClosereturnvoid(notboolas the issue draft suggested). The existingUdpSenderdoes not check the return values ofsocket()orclose(), so adding a bool return would introduce behaviour that has no caller and no tests. Error handling is deferred to Epic E12: Error Handling #31.SendToreturnsbool, preserving the existingsendto(...) >= 0semantics.struct sockaddr_instays inUdpSender(which owns the resolver) and is passed through toSendToon each call. The Datagram is stateless w.r.t. addressing. The POSIX sockaddr_in type still leaks through the Datagram interface — same situation as the Resolver — hiding it is deferred until Winsock or connection-lifecycle work demands it.GetAddrInfoResolver. One Datagram per UdpSender. Limitation tracked in fix: validate numeric CLI args for --max-files and --max-file-size #110 / memory.Pure refactor — no new functionality, no behaviour change.
Test Evidence
New tests —
Tests/SolidSyslogPosixDatagramTest.cpp: 14 tests drove the PosixDatagram implementation via strict TDD (red/green for each behaviour). Covers Open (socket called, AF_INET, SOCK_DGRAM), SendTo (called once, buffer/length/flags/fd/address/addrlen propagation, return value on success and failure), and Close (called with correct fd).Existing tests preserved —
SolidSyslogUdpSenderTest.cpp(22 tests across 3 test groups) remain unchanged in their assertions and test bodies. Only the setup/teardown wiring changed to create/destroy aPosixDatagram. BecauseSocketFakeintercepts at the libc level, all existing assertions (SocketFake_SocketCallCount,SocketFake_LastPort,SocketFake_CloseCallCount, etc.) continue to pass, proving the refactor preserves behaviour.Local CI — all presets green:
debug,clang-debug,sanitize: 498 tests passcoverage: 100% on new files (SolidSyslogDatagram.c, SolidSyslogPosixDatagram.c); 99.9% overall (pre-existing gap in SolidSyslogFileStore.c, unrelated)tidy,cppcheck: cleanclang-format --dry-run --Werror: cleanAreas Affected
New:
Interface/SolidSyslogDatagram.h,SolidSyslogDatagramDefinition.h,SolidSyslogPosixDatagram.hSource/SolidSyslogDatagram.c,SolidSyslogPosixDatagram.cTests/SolidSyslogPosixDatagramTest.cppModified:
Interface/SolidSyslogUdpSender.h— config gainsdatagramfieldSource/SolidSyslogUdpSender.c— delegates socket operations to injected Datagram; removedfdfieldTests/SolidSyslogUdpSenderTest.cpp— setup/teardown wire in PosixDatagramTests/Example/ExampleServiceThreadTest.cpp— same wiring updateExample/SingleTask/SolidSyslogExample.c,Example/Threaded/main.c— create/destroy PosixDatagramSource/CMakeLists.txt,Tests/CMakeLists.txt— add new source filesNot affected:
TcpSender— S13.8 will extract the Stream abstraction for TCP connection operations.Summary by CodeRabbit
Release Notes
New Features
Tests