feat: S13.5 Windows example executable#153
Merged
Merged
Conversation
Adds the SolidSyslogWindowsExample CMake target — wires WinsockResolver + WinsockDatagram + WindowsClock/Hostname/ProcessId + WSAStartup/Cleanup into a fire-and-forget UDP example. Output binary is named SolidSyslogExample.exe (same as the POSIX target) so BDD scenarios stay platform-agnostic. ExampleAppName_Set now treats both '/' and '\' as path separators; needed because Windows argv[0] uses backslashes. TDD'd in Tests/Example/ExampleAppNameTest.cpp with regression coverage for forward-slash, no-separator, and mixed-separator paths. SolidSyslogUdpSender.c is platform-agnostic glue — moved out of the SOLIDSYSLOG_POSIX-only group into the SOLIDSYSLOG_POSIX OR SOLIDSYSLOG_WINSOCK group so the Windows target can link against it. Refs #129 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Windows/Winsock example and adjusts CMake to build the Example and UDP sender for Winsock when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant App as SolidSyslogExample (Windows)
participant Winsock as Winsock (WSAStartup)
participant Resolver as WinsockResolver
participant Datagram as WinsockDatagram
participant Sender as UdpSender
participant Syslog as SolidSyslog Core
rect rgba(200,200,255,0.5)
User->>App: run(argc, argv)
App->>Winsock: WSAStartup()
Winsock-->>App: success
App->>Resolver: create resolver(127.0.0.1:5514)
Resolver-->>Datagram: provide endpoint
App->>Datagram: create datagram
Datagram-->>Sender: hand datagram
App->>Syslog: SolidSyslog_Create(config{sender, sd, clocks, getters})
Syslog-->>App: started
User->>App: interactive input
App->>Syslog: send formatted messages
Syslog-->>Sender: transmit UDP packets
User->>App: exit
App->>Syslog: SolidSyslog_Destroy()
App->>Winsock: WSACleanup()
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (✔️ 535 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Tests/Example/ExampleAppNameTest.cpp (1)
31-57: Extract repeated test literals intoTEST_*constants.These cases are good, but moving inline literals to named
TEST_*constants will improve consistency and future edits.💡 Suggested refactor
+namespace +{ +constexpr const char* TEST_PATH_BACKSLASH = "dir\\binary.exe"; +constexpr const char* TEST_NAME_BINARY_EXE = "binary.exe"; +constexpr const char* TEST_PATH_FORWARD_SLASH = "/usr/local/bin/example"; +constexpr const char* TEST_NAME_EXAMPLE = "example"; +constexpr const char* TEST_PATH_NO_SEPARATOR = "example"; +constexpr const char* TEST_PATH_MIXED = "C:\\msys64\\home/user/example.exe"; +} + TEST(ExampleAppName, BackslashSeparatorExtractsBaseName) { - ExampleAppName_Set("dir\\binary.exe"); + ExampleAppName_Set(TEST_PATH_BACKSLASH); ExampleAppName_Get(formatter); - STRCMP_EQUAL("binary.exe", formatted()); + STRCMP_EQUAL(TEST_NAME_BINARY_EXE, formatted()); } TEST(ExampleAppName, ForwardSlashSeparatorExtractsBaseName) { - ExampleAppName_Set("/usr/local/bin/example"); + ExampleAppName_Set(TEST_PATH_FORWARD_SLASH); ExampleAppName_Get(formatter); - STRCMP_EQUAL("example", formatted()); + STRCMP_EQUAL(TEST_NAME_EXAMPLE, formatted()); } TEST(ExampleAppName, NoSeparatorReturnsWholeArgument) { - ExampleAppName_Set("example"); + ExampleAppName_Set(TEST_PATH_NO_SEPARATOR); ExampleAppName_Get(formatter); - STRCMP_EQUAL("example", formatted()); + STRCMP_EQUAL(TEST_NAME_EXAMPLE, formatted()); } TEST(ExampleAppName, MixedSeparatorsUseRightmost) { - ExampleAppName_Set("C:\\msys64\\home/user/example.exe"); + ExampleAppName_Set(TEST_PATH_MIXED); ExampleAppName_Get(formatter); - STRCMP_EQUAL("example.exe", formatted()); + STRCMP_EQUAL(TEST_NAME_BINARY_EXE, formatted()); }As per coding guidelines: "Use hard-coded 'test default' values that are obviously fake (e.g.
TestHost,42,2009-03-23T00:00:00.000Z) and name themTEST_*constants".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/Example/ExampleAppNameTest.cpp` around lines 31 - 57, The tests embed repeated string literals; extract them into clearly named TEST_* constants (e.g., TEST_PATH_BACKSLASH = "dir\\binary.exe", TEST_EXPECT_BACKSLASH = "binary.exe", TEST_PATH_FORWARD = "/usr/local/bin/example", TEST_EXPECT_FORWARD = "example", TEST_PATH_NOSEP = "example", TEST_EXPECT_NOSEP = "example", TEST_PATH_MIXED = "C:\\msys64\\home/user/example.exe", TEST_EXPECT_MIXED = "example.exe"), define these constants near the top of the test file, then replace the inline literals in the ExampleAppName_Set calls and STRCMP_EQUAL expected args in the tests (BackslashSeparatorExtractsBaseName, ForwardSlashSeparatorExtractsBaseName, NoSeparatorReturnsWholeArgument, MixedSeparatorsUseRightmost) to use the corresponding TEST_* constants to improve consistency and ease future edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Example/CMakeLists.txt`:
- Around line 1-42: The POSIX and Windows example targets can both be created
and currently end up colliding because SolidSyslogExample (add_executable name)
and SolidSyslogWindowsExample (set via set_target_properties OUTPUT_NAME
"SolidSyslogExample") produce the same output name; make the selection mutually
exclusive or ensure unique output names: either change the two independent
if(...) blocks into a single if(SOLIDSYSLOG_POSIX) ...
elseif(SOLIDSYSLOG_WINSOCK AND HAVE_WINDOWS_PLATFORM) ... endif so only one
target is emitted, or keep the separate conditionals but change the Windows
target's OUTPUT_NAME (or the POSIX target name) so SolidSyslogExample and
SolidSyslogWindowsExample do not share the same OUTPUT_NAME property.
In `@Example/Common/ExampleAppName.c`:
- Around line 10-13: The code compares pointers forwardSlash and backSlash with
'>' even when one may be NULL (undefined behavior); change the logic in the
block that computes separator/appName (variables forwardSlash, backSlash,
separator, appName, argv0) to first handle NULL cases explicitly (if both NULL
set appName=argv0; if one is NULL pick the other), and only perform a relational
comparison when both forwardSlash and backSlash are non-NULL to select the later
separator.
In `@Example/Windows/SolidSyslogWindowsExample.c`:
- Around line 45-55: The function SolidSyslogWindowsExample_Run uses argv[0]
without validating argc/argv; add an early guard at the start of
SolidSyslogWindowsExample_Run to ensure argv is non-NULL and argc > 0 before
calling ExampleAppName_Set(argv[0]) and return an error code (e.g., 1) or handle
accordingly if the check fails so we avoid undefined behavior for non-main
callers.
- Around line 50-53: After calling WSAStartup(MAKEWORD(2,2), &wsaData) in your
initialization code, verify that the negotiated version in wsaData.wVersion
equals MAKEWORD(2,2); if not, call WSACleanup() and return an error instead of
proceeding. Locate the WSAStartup call and add a check against wsaData.wVersion
(compare to MAKEWORD(2,2)), invoke WSACleanup() on failure, and return a nonzero
status so the program doesn't assume Winsock 2.2 is available.
- Around line 57-66: The sequence of Create calls
(SolidSyslogWinsockResolver_Create, SolidSyslogWinsockDatagram_Create,
SolidSyslogUdpSender_Create, SolidSyslogNullBuffer_Create,
SolidSyslogNullStore_Create, SolidSyslogAtomicCounter_Create,
SolidSyslogMetaSd_Create, SolidSyslogTimeQualitySd_Create,
SolidSyslogOriginSd_Create) must have staged NULL checks: after each Create,
check the returned pointer for NULL before calling the next constructor; if any
allocation fails, tear down/free any previously created objects (resolver,
datagram, sender, buffer, store, counter, metaSd, timeQuality, originSd as
applicable) and return/exit with an error to avoid passing NULL into
SolidSyslogUdpSender_Create or later calls. Ensure the check before calling
SolidSyslogUdpSender_Create verifies resolver and datagram are non-NULL and
similarly guard subsequent Create calls for structured data and buffer/store.
---
Nitpick comments:
In `@Tests/Example/ExampleAppNameTest.cpp`:
- Around line 31-57: The tests embed repeated string literals; extract them into
clearly named TEST_* constants (e.g., TEST_PATH_BACKSLASH = "dir\\binary.exe",
TEST_EXPECT_BACKSLASH = "binary.exe", TEST_PATH_FORWARD =
"/usr/local/bin/example", TEST_EXPECT_FORWARD = "example", TEST_PATH_NOSEP =
"example", TEST_EXPECT_NOSEP = "example", TEST_PATH_MIXED =
"C:\\msys64\\home/user/example.exe", TEST_EXPECT_MIXED = "example.exe"), define
these constants near the top of the test file, then replace the inline literals
in the ExampleAppName_Set calls and STRCMP_EQUAL expected args in the tests
(BackslashSeparatorExtractsBaseName, ForwardSlashSeparatorExtractsBaseName,
NoSeparatorReturnsWholeArgument, MixedSeparatorsUseRightmost) to use the
corresponding TEST_* constants to improve consistency and ease future edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4edad6f6-b3a3-4930-bf09-e3dd70972868
📒 Files selected for processing (9)
CMakeLists.txtExample/CMakeLists.txtExample/Common/ExampleAppName.cExample/Windows/SolidSyslogWindowsExample.cExample/Windows/SolidSyslogWindowsExample.hExample/Windows/main.cSource/CMakeLists.txtTests/Example/CMakeLists.txtTests/Example/ExampleAppNameTest.cpp
Comment on lines
+45
to
+55
| int SolidSyslogWindowsExample_Run(int argc, char* argv[]) | ||
| { | ||
| (void) argc; | ||
|
|
||
| WSADATA wsaData; | ||
| if (WSAStartup(MAKEWORD(2, 2), &wsaData) != 0) | ||
| { | ||
| return 1; | ||
| } | ||
|
|
||
| ExampleAppName_Set(argv[0]); |
Contributor
There was a problem hiding this comment.
Guard argv[0] before use in SolidSyslogWindowsExample_Run.
Line 55 dereferences argv[0] without validating argc/argv. Add an early argument guard to avoid undefined behavior for non-main callers.
Proposed fix
int SolidSyslogWindowsExample_Run(int argc, char* argv[])
{
- (void) argc;
+ if ((argc <= 0) || (argv == NULL) || (argv[0] == NULL))
+ {
+ return 1;
+ }
WSADATA wsaData;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Example/Windows/SolidSyslogWindowsExample.c` around lines 45 - 55, The
function SolidSyslogWindowsExample_Run uses argv[0] without validating
argc/argv; add an early guard at the start of SolidSyslogWindowsExample_Run to
ensure argv is non-NULL and argc > 0 before calling ExampleAppName_Set(argv[0])
and return an error code (e.g., 1) or handle accordingly if the check fails so
we avoid undefined behavior for non-main callers.
Owner
Author
There was a problem hiding this comment.
Declining. C11 §5.1.2.2.1 guarantees that when called via main, argc > 0 and argv[0] points to a valid string (or to an empty string if no program name was available). The Linux SingleTask example does the same ExampleAppName_Set(argv[0]) without a guard (SolidSyslogExample.c:30). Project policy in CLAUDE.md is explicit: "Don't add error handling, fallbacks, or validation for scenarios that can't happen. Trust internal code and framework guarantees." Adding a guard here would create asymmetry with the sibling example for no real-world failure mode.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — the C11 standard guarantee, the consistent pattern with the POSIX example, and the explicit project policy all make the reasoning clear. I'll drop this suggestion.
(∩ ` -´)⊃━ ✿
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:45-55
Timestamp: 2026-04-18T07:18:35.201Z
Learning: In `DavidCozens/solid-syslog`, example entry-point functions (e.g., `SolidSyslogWindowsExample_Run` in `Example/Windows/SolidSyslogWindowsExample.c` and `SolidSyslogExample_Run` in `Example/SingleTask/SolidSyslogExample.c`) intentionally omit argc/argv NULL guards before calling `ExampleAppName_Set(argv[0])`. C11 §5.1.2.2.1 guarantees `argc > 0` and a valid `argv[0]` when invoked via `main`, and project policy (CLAUDE.md) forbids defensive checks for scenarios that cannot happen. Do not flag missing argc/argv guards in these example entry points in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:57-71
Timestamp: 2026-04-09T20:59:09.391Z
Learning: In `Source/SolidSyslogTcpSender.c`, error handling for `connect()` failure and `send()` failure (including partial writes, SIGPIPE mitigation, and reconnection logic) is intentionally deferred to story S15.2. In the current implementation (S15.1), `tcp->connected` is set to `true` unconditionally after `connect()`, and `send()` return values are ignored. S15.2 will test-drive: connect failure → return false (and do NOT set `tcp->connected = true`), send failure → close socket and reconnect, and a `SendAll()` helper that retries on partial writes.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 137
File: Source/SolidSyslogUdpSender.c:0-0
Timestamp: 2026-04-15T21:12:56.960Z
Learning: In `Source/SolidSyslogUdpSender.c`, NULL checks for `config` and `config->resolver` in `SolidSyslogUdpSender_Create` are intentionally deferred to the error-handling epic (Epic `#31`). The function currently calls `SolidSyslogResolver_Resolve()` dispatch (not the vtable directly) and includes `SolidSyslogResolver.h`. Do not flag missing NULL guards on config/resolver in this function in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the extern function-pointer seam variables in `Platform/Windows/Source/SolidSyslogWinsockDatagramInternal.h` and `Platform/Windows/Source/SolidSyslogWinsockResolverInternal.h` (`Winsock_socket`, `Winsock_sendto`, `Winsock_closesocket`, `Winsock_getaddrinfo`, `Winsock_freeaddrinfo`) intentionally use underscore-based naming (mirroring the real Winsock API names) to preserve visual clarity at `UT_PTR_SET` call sites. Do not flag these names as non-camelCase in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 149
File: Platform/Windows/Source/SolidSyslogWinsockResolver.c:67-72
Timestamp: 2026-04-17T20:07:36.721Z
Learning: In `Platform/Windows/Source/SolidSyslogWinsockResolver.c` (and its POSIX mirror `Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c`), the dereference of `info->ai_addr` without NULL/addrlen validation, and the unchecked cast of `getPort()` to `uint16_t` before `htons()`, are intentional. The resolver trusts its injected `getHost`/`getPort` callbacks by design (SRP); port-range and `ai_addr` defensive checks belong at the config/caller layer. Any future hardening must be a coordinated change across both resolvers together. Do not flag these as bugs or missing validation in future reviews of either resolver file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 109
File: Example/Common/ExampleCommandLine.c:83-88
Timestamp: 2026-04-13T09:52:59.823Z
Learning: In `Example/Common/ExampleCommandLine.c`, the `strtol`-without-endptr/sign-validation pattern for `OPT_MAX_FILES` and `OPT_MAX_FILE_SIZE` (around lines 83–88) is a known pre-existing deferral. The values are clamped downstream in `SolidSyslogFileStore_Create`, so there is no runtime safety issue. Parse-time validation (endptr + non-negative check, returning 1 on bad input) is backlogged for a follow-up PR after PR `#109` merges. Do not re-flag this as a new issue in future reviews of these two `case` branches.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 131
File: Tests/Support/SafeStringWindows.c:5-8
Timestamp: 2026-04-15T15:53:38.455Z
Learning: In `Tests/Support/SafeStringWindows.c` of `DavidCozens/solid-syslog`, the `SafeString_Copy` Windows implementation intentionally omits a `destSize == 0` guard (unlike `SafeStringStandard.c`). The design goal is to let `strncpy_s` invoke the MSVC invalid parameter handler on misuse, acting as a fail-fast mechanism in test code. Do not flag this asymmetry as a defect in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Source/SolidSyslogFileStore.c:124-128
Timestamp: 2026-04-11T22:49:26.725Z
Learning: In `Source/SolidSyslogFileStore.c`, the return values of `OpenWriteFile()` and `OpenReadFile()` are intentionally unchecked in `SolidSyslogFileStore_Create` (around lines 124–128) and `RotateToNextFile` (around lines 394–400). Guarding against open failures and aborting the create/rotation flow is deferred to the error-handling epic, tracked in `project_robustness_backlog.md` under "FileStore error handling (PR `#103`)". Do not flag missing open-failure guards in these functions in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the `_CRT_SECURE_NO_WARNINGS` compile definition is a known temporary stopgap for MSVC CRT deprecation warnings in test fakes. Issue `#126` (S13.3) will introduce a `SafeString` abstraction (Windows impl wraps `_s` variants; POSIX falls through to standard functions); once test fakes migrate, this suppression is removed. Production code is already SDL-clean. Do not flag this definition as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4200` (flexible array member) is a known stopgap. Issue `#125` (S13.2) will replace `char buffer[]` with the `char buffer[1]` pre-C99 idiom and remove this suppression. Do not flag `/wd4200` in future reviews of this file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4297` (exception thrown from `extern "C"` function) is intentional and permanent test-infrastructure design: C fakes require C linkage, and `TestAssert_Fail` throws to propagate failures through CppUTest. Issue `#125` will add an inline comment documenting this justification. Do not flag `/wd4297` as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 113
File: Source/SolidSyslogFileStore.c:0-0
Timestamp: 2026-04-13T22:11:29.377Z
Learning: In `Source/SolidSyslogFileStore.c`, `FormatFilename()` does not reserve space for the 2-digit padded sequence and ".log" suffix before writing `pathPrefix`. Writes past the buffer are silently stopped by the `WriteChar` bounds check in `SolidSyslogFormatter`, so there is no overflow risk. The filename-collapsing behaviour for very long `pathPrefix` values is a pre-existing deferral, tracked under S12.3 (`#87`) for comprehensive overflow protection and return-value checking. Do not re-flag this in future reviews of `FormatFilename`.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Source/SolidSyslogFileStore.c:174-199
Timestamp: 2026-04-11T21:59:19.899Z
Learning: In `Source/SolidSyslogFileStore.c`, the `ScanForExistingFiles()` function has a known bug: when log files wrap past sequence 99→00, the linear 0–99 scan incorrectly identifies the oldest/newest sequence (numerically first file may actually be the newest). This is a documented deferral in the project notes and will be addressed in a dedicated follow-up story for gap-detection on startup recovery. Do not flag this as a new bug in future reviews of this function.
Learnt from: CR
Repo: DavidCozens/solid-syslog PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T21:25:03.776Z
Learning: Applies to Example/Common/**/*.{c,cpp,cc,cxx,h,hpp} : Shared example code (CLI parsing, app name, UDP/TCP config, service thread) goes in `Example/Common/`
Learnt from: CR
Repo: DavidCozens/solid-syslog PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T21:25:03.776Z
Learning: Applies to Source/**/*.{c,cpp,cc,cxx} : Production code must never use Microsoft SDL banned functions (`strncpy`, `sprintf`, `sscanf`, `strtok`, etc.); use `SolidSyslogFormatter` for string building and `strtol` for parsing
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 137
File: Interface/SolidSyslogResolverDefinition.h:12-12
Timestamp: 2026-04-15T21:12:31.145Z
Learning: In `Interface/SolidSyslogResolverDefinition.h` (and the corresponding `Source/SolidSyslogResolver.c`), the `Resolve` function pointer returns `void` intentionally. This is a direct extraction of the pre-existing behaviour in `SolidSyslogUdpSender` and `SolidSyslogTcpSender`, neither of which checked resolution success before PR `#137`. Adding a boolean/error return to `Resolve` and propagating failures through sender create/connect paths is deferred to the error-handling epic (Epic `#31`). Do not flag the `void` return on `SolidSyslogResolver.Resolve` as a contract gap in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the test helper methods `Resolve()` and `Result()` in `Tests/SolidSyslogWinsockResolverTest.cpp` intentionally use PascalCase to mirror the convention in the POSIX counterpart `Tests/SolidSyslogGetAddrInfoResolverTest.cpp`. Do not flag PascalCase test helper methods in Winsock or POSIX resolver tests in future reviews.
Comment on lines
+50
to
+53
| if (WSAStartup(MAKEWORD(2, 2), &wsaData) != 0) | ||
| { | ||
| return 1; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to current Microsoft Winsock documentation for WSAStartup, should callers verify wsaData.wVersion matches the requested 2.2 after a successful WSAStartup(MAKEWORD(2,2), ...) call?
💡 Result:
Yes, according to the current Microsoft Winsock documentation, callers should verify that wsaData.wVersion matches the requested version 2.2 after a successful WSAStartup(MAKEWORD(2,2), ...) call. The official documentation explicitly recommends this check in its example code and explanatory comments to confirm the Winsock DLL supports the requested version, even though the call succeeds. If the version does not match (e.g., LOBYTE(wsaData.wVersion) != 2 || HIBYTE(wsaData.wVersion) != 2), the application should call WSACleanup and handle it as an unusable DLL.
Citations:
- 1: https://learn.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-wsastartup
- 2: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsastartup
- 3: https://learn.microsoft.com/en-us/windows/win32/api/winsock2/ns-winsock2-wsadata
- 4: https://learn.microsoft.com/en-us/windows/win32/api/winsock/ns-winsock-wsadata
Validate negotiated Winsock version after WSAStartup.
WSAStartup returning 0 does not guarantee version 2.2 was negotiated. Check wsaData.wVersion and call WSACleanup() before returning if it is not 2.2.
Proposed fix
if (WSAStartup(MAKEWORD(2, 2), &wsaData) != 0)
{
return 1;
}
+ if ((LOBYTE(wsaData.wVersion) != 2) || (HIBYTE(wsaData.wVersion) != 2))
+ {
+ WSACleanup();
+ return 1;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (WSAStartup(MAKEWORD(2, 2), &wsaData) != 0) | |
| { | |
| return 1; | |
| } | |
| if (WSAStartup(MAKEWORD(2, 2), &wsaData) != 0) | |
| { | |
| return 1; | |
| } | |
| if ((LOBYTE(wsaData.wVersion) != 2) || (HIBYTE(wsaData.wVersion) != 2)) | |
| { | |
| WSACleanup(); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Example/Windows/SolidSyslogWindowsExample.c` around lines 50 - 53, After
calling WSAStartup(MAKEWORD(2,2), &wsaData) in your initialization code, verify
that the negotiated version in wsaData.wVersion equals MAKEWORD(2,2); if not,
call WSACleanup() and return an error instead of proceeding. Locate the
WSAStartup call and add a check against wsaData.wVersion (compare to
MAKEWORD(2,2)), invoke WSACleanup() on failure, and return a nonzero status so
the program doesn't assume Winsock 2.2 is available.
Owner
Author
There was a problem hiding this comment.
Declining. Winsock 2.2 has been available on every Windows release since Windows 95 OSR2 (1996); we target windows-2025 and Windows 10+ devboxes. The version-mismatch check is defensive against a configuration that doesn't exist on any supported environment. CLAUDE.md guidance: "Trust internal code and framework guarantees. Only validate at system boundaries (user input, external APIs)." Microsoft's recommendation makes sense for a general-purpose library targeting unknown environments — for an example program with a known-modern target, it's noise.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — that's a well-reasoned call. Winsock 2.2 is a given on any supported Windows target, and the CLAUDE.md "trust internal guarantees" principle applies cleanly here. I'll drop this suggestion.
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:50-53
Timestamp: 2026-04-18T07:18:41.688Z
Learning: In `Example/Windows/SolidSyslogWindowsExample.c` of `DavidCozens/solid-syslog`, the `wsaData.wVersion` version-match check after `WSAStartup(MAKEWORD(2,2), &wsaData)` is intentionally omitted. The project targets Windows 10+ / windows-2025 where Winsock 2.2 is guaranteed to be available; CLAUDE.md guidance says to trust internal code and framework guarantees. Do not flag the missing `LOBYTE/HIBYTE(wsaData.wVersion)` version check in future reviews of this file.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/CMakeLists.txt:1-40
Timestamp: 2026-04-18T07:18:19.522Z
Learning: In `DavidCozens/solid-syslog` `Example/CMakeLists.txt`, `SOLIDSYSLOG_POSIX` and `SOLIDSYSLOG_WINSOCK` are independently auto-detected (POSIX from `<sys/socket.h>`, Winsock from `<winsock2.h>`). On MinGW/Cygwin both can be ON simultaneously. The two example targets (`SolidSyslogExample` and `SolidSyslogWindowsExample` with `OUTPUT_NAME "SolidSyslogExample"`) are intentionally made mutually exclusive via `if(SOLIDSYSLOG_POSIX) ... elseif(SOLIDSYSLOG_WINSOCK AND HAVE_WINDOWS_PLATFORM) ... endif()` to prevent CMake output-name collisions. Do not suggest independent `if` blocks for these two targets in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:57-71
Timestamp: 2026-04-09T20:59:09.391Z
Learning: In `Source/SolidSyslogTcpSender.c`, error handling for `connect()` failure and `send()` failure (including partial writes, SIGPIPE mitigation, and reconnection logic) is intentionally deferred to story S15.2. In the current implementation (S15.1), `tcp->connected` is set to `true` unconditionally after `connect()`, and `send()` return values are ignored. S15.2 will test-drive: connect failure → return false (and do NOT set `tcp->connected = true`), send failure → close socket and reconnect, and a `SendAll()` helper that retries on partial writes.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 137
File: Source/SolidSyslogUdpSender.c:0-0
Timestamp: 2026-04-15T21:12:56.960Z
Learning: In `Source/SolidSyslogUdpSender.c`, NULL checks for `config` and `config->resolver` in `SolidSyslogUdpSender_Create` are intentionally deferred to the error-handling epic (Epic `#31`). The function currently calls `SolidSyslogResolver_Resolve()` dispatch (not the vtable directly) and includes `SolidSyslogResolver.h`. Do not flag missing NULL guards on config/resolver in this function in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 149
File: Platform/Windows/Source/SolidSyslogWinsockResolver.c:67-72
Timestamp: 2026-04-17T20:07:36.721Z
Learning: In `Platform/Windows/Source/SolidSyslogWinsockResolver.c` (and its POSIX mirror `Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c`), the dereference of `info->ai_addr` without NULL/addrlen validation, and the unchecked cast of `getPort()` to `uint16_t` before `htons()`, are intentional. The resolver trusts its injected `getHost`/`getPort` callbacks by design (SRP); port-range and `ai_addr` defensive checks belong at the config/caller layer. Any future hardening must be a coordinated change across both resolvers together. Do not flag these as bugs or missing validation in future reviews of either resolver file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the extern function-pointer seam variables in `Platform/Windows/Source/SolidSyslogWinsockDatagramInternal.h` and `Platform/Windows/Source/SolidSyslogWinsockResolverInternal.h` (`Winsock_socket`, `Winsock_sendto`, `Winsock_closesocket`, `Winsock_getaddrinfo`, `Winsock_freeaddrinfo`) intentionally use underscore-based naming (mirroring the real Winsock API names) to preserve visual clarity at `UT_PTR_SET` call sites. Do not flag these names as non-camelCase in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4297` (exception thrown from `extern "C"` function) is intentional and permanent test-infrastructure design: C fakes require C linkage, and `TestAssert_Fail` throws to propagate failures through CppUTest. Issue `#125` will add an inline comment documenting this justification. Do not flag `/wd4297` as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 131
File: Tests/Support/SafeStringWindows.c:5-8
Timestamp: 2026-04-15T15:53:38.455Z
Learning: In `Tests/Support/SafeStringWindows.c` of `DavidCozens/solid-syslog`, the `SafeString_Copy` Windows implementation intentionally omits a `destSize == 0` guard (unlike `SafeStringStandard.c`). The design goal is to let `strncpy_s` invoke the MSVC invalid parameter handler on misuse, acting as a fail-fast mechanism in test code. Do not flag this asymmetry as a defect in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the `_CRT_SECURE_NO_WARNINGS` compile definition is a known temporary stopgap for MSVC CRT deprecation warnings in test fakes. Issue `#126` (S13.3) will introduce a `SafeString` abstraction (Windows impl wraps `_s` variants; POSIX falls through to standard functions); once test fakes migrate, this suppression is removed. Production code is already SDL-clean. Do not flag this definition as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4200` (flexible array member) is a known stopgap. Issue `#125` (S13.2) will replace `char buffer[]` with the `char buffer[1]` pre-C99 idiom and remove this suppression. Do not flag `/wd4200` in future reviews of this file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 109
File: Example/Common/ExampleCommandLine.c:83-88
Timestamp: 2026-04-13T09:52:59.823Z
Learning: In `Example/Common/ExampleCommandLine.c`, the `strtol`-without-endptr/sign-validation pattern for `OPT_MAX_FILES` and `OPT_MAX_FILE_SIZE` (around lines 83–88) is a known pre-existing deferral. The values are clamped downstream in `SolidSyslogFileStore_Create`, so there is no runtime safety issue. Parse-time validation (endptr + non-negative check, returning 1 on bad input) is backlogged for a follow-up PR after PR `#109` merges. Do not re-flag this as a new issue in future reviews of these two `case` branches.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the test helper methods `Resolve()` and `Result()` in `Tests/SolidSyslogWinsockResolverTest.cpp` intentionally use PascalCase to mirror the convention in the POSIX counterpart `Tests/SolidSyslogGetAddrInfoResolverTest.cpp`. Do not flag PascalCase test helper methods in Winsock or POSIX resolver tests in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Source/SolidSyslogFileStore.c:174-199
Timestamp: 2026-04-11T21:59:19.899Z
Learning: In `Source/SolidSyslogFileStore.c`, the `ScanForExistingFiles()` function has a known bug: when log files wrap past sequence 99→00, the linear 0–99 scan incorrectly identifies the oldest/newest sequence (numerically first file may actually be the newest). This is a documented deferral in the project notes and will be addressed in a dedicated follow-up story for gap-detection on startup recovery. Do not flag this as a new bug in future reviews of this function.
Learnt from: CR
Repo: DavidCozens/solid-syslog PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T21:25:03.776Z
Learning: Applies to Example/Common/**/*.{c,cpp,cc,cxx,h,hpp} : Shared example code (CLI parsing, app name, UDP/TCP config, service thread) goes in `Example/Common/`
Learnt from: CR
Repo: DavidCozens/solid-syslog PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T21:25:03.776Z
Learning: Applies to Source/**/*.{c,cpp,cc,cxx} : Production code must never use Microsoft SDL banned functions (`strncpy`, `sprintf`, `sscanf`, `strtok`, etc.); use `SolidSyslogFormatter` for string building and `strtol` for parsing
Comment on lines
+57
to
+66
| struct SolidSyslogResolver* resolver = SolidSyslogWinsockResolver_Create(GetHost, GetPort); | ||
| struct SolidSyslogDatagram* datagram = SolidSyslogWinsockDatagram_Create(); | ||
| struct SolidSyslogUdpSenderConfig udpConfig = {.resolver = resolver, .datagram = datagram}; | ||
| struct SolidSyslogSender* sender = SolidSyslogUdpSender_Create(&udpConfig); | ||
| struct SolidSyslogBuffer* buffer = SolidSyslogNullBuffer_Create(sender); | ||
| struct SolidSyslogStore* store = SolidSyslogNullStore_Create(); | ||
| struct SolidSyslogAtomicCounter* counter = SolidSyslogAtomicCounter_Create(); | ||
| struct SolidSyslogStructuredData* metaSd = SolidSyslogMetaSd_Create(counter); | ||
| struct SolidSyslogStructuredData* timeQuality = SolidSyslogTimeQualitySd_Create(GetTimeQuality); | ||
| struct SolidSyslogStructuredData* originSd = SolidSyslogOriginSd_Create("SolidSyslogExample", "0.7.0"); |
Contributor
There was a problem hiding this comment.
Add staged NULL checks between dependent Create calls.
This block chains dependent allocations/constructors without intermediate checks. If an earlier Create fails, a later call can receive NULL dependencies and fail unpredictably.
Suggested pattern (apply through the remaining create chain)
struct SolidSyslogResolver* resolver = SolidSyslogWinsockResolver_Create(GetHost, GetPort);
+ if (resolver == NULL)
+ {
+ WSACleanup();
+ return 1;
+ }
struct SolidSyslogDatagram* datagram = SolidSyslogWinsockDatagram_Create();
+ if (datagram == NULL)
+ {
+ SolidSyslogWinsockResolver_Destroy();
+ WSACleanup();
+ return 1;
+ }
struct SolidSyslogUdpSenderConfig udpConfig = {.resolver = resolver, .datagram = datagram};
struct SolidSyslogSender* sender = SolidSyslogUdpSender_Create(&udpConfig);
+ if (sender == NULL)
+ {
+ SolidSyslogWinsockDatagram_Destroy();
+ SolidSyslogWinsockResolver_Destroy();
+ WSACleanup();
+ return 1;
+ }Based on learnings, SolidSyslogUdpSender_Create currently has intentionally deferred NULL guards for config/config->resolver, so this callsite should guard dependency creation failures before passing inputs onward.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Example/Windows/SolidSyslogWindowsExample.c` around lines 57 - 66, The
sequence of Create calls (SolidSyslogWinsockResolver_Create,
SolidSyslogWinsockDatagram_Create, SolidSyslogUdpSender_Create,
SolidSyslogNullBuffer_Create, SolidSyslogNullStore_Create,
SolidSyslogAtomicCounter_Create, SolidSyslogMetaSd_Create,
SolidSyslogTimeQualitySd_Create, SolidSyslogOriginSd_Create) must have staged
NULL checks: after each Create, check the returned pointer for NULL before
calling the next constructor; if any allocation fails, tear down/free any
previously created objects (resolver, datagram, sender, buffer, store, counter,
metaSd, timeQuality, originSd as applicable) and return/exit with an error to
avoid passing NULL into SolidSyslogUdpSender_Create or later calls. Ensure the
check before calling SolidSyslogUdpSender_Create verifies resolver and datagram
are non-NULL and similarly guard subsequent Create calls for structured data and
buffer/store.
Owner
Author
There was a problem hiding this comment.
Declining. The Linux SingleTask example follows the same chain-without-checks pattern (SolidSyslogExample.c:38-47). The library Create functions return pointers to static instances — they can't fail at the constructor level. Where Create can fail (e.g. resolver cannot resolve a host, see S12.11 work in #144), the library reports honestly through the vtable at use-time, not the constructor. Adding NULL checks here would create asymmetry with the Linux example and validate scenarios the library design rules out. Same root rationale as the argv[0] reply.
Contributor
There was a problem hiding this comment.
@DavidCozens, understood — the static-instance contract makes NULL checks both unnecessary and misleading here. I'll accept this and note the design for future reviews.
(ᓀ ᓀ)
✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:57-66
Timestamp: 2026-04-18T07:18:55.197Z
Learning: In `DavidCozens/solid-syslog`, all library `Create` functions (e.g. `SolidSyslogWinsockResolver_Create`, `SolidSyslogWinsockDatagram_Create`, `SolidSyslogUdpSender_Create`, `SolidSyslogNullBuffer_Create`, `SolidSyslogNullStore_Create`, `SolidSyslogAtomicCounter_Create`, `SolidSyslogMetaSd_Create`, `SolidSyslogTimeQualitySd_Create`, `SolidSyslogOriginSd_Create`) return pointers to static instances and cannot fail at the constructor level. Where a runtime failure can occur (e.g. host resolution in the resolver), it is reported through the vtable at use-time, not at construction time. Therefore, NULL checks after `Create` calls in example entry points (both `Example/SingleTask/SolidSyslogExample.c` and `Example/Windows/SolidSyslogWindowsExample.c`) are intentionally omitted — they would guard impossible scenarios and create asymmetry between platform examples. Do not flag missing NULL checks on `Create` call chains in example files in future reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/Windows/SolidSyslogWindowsExample.c:45-55
Timestamp: 2026-04-18T07:18:35.201Z
Learning: In this repository’s example entry-point C files under `Example/` (e.g., `SolidSyslogWindowsExample.c`, `SolidSyslogExample.c`), it is intentional to omit `argc/argv` NULL/length guards before using `argv[0]` (e.g., calling `ExampleAppName_Set(argv[0])`). C guarantees that when invoked via `main`, `argc > 0` and `argv[0]` is a valid string (§5.1.2.2.1), and the project policy (CLAUDE.md) forbids defensive checks for impossible scenarios. Therefore, do not raise review findings for missing `argc/argv` guards in these example entry points.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 153
File: Example/CMakeLists.txt:1-40
Timestamp: 2026-04-18T07:18:19.522Z
Learning: In `DavidCozens/solid-syslog` `Example/CMakeLists.txt`, `SOLIDSYSLOG_POSIX` and `SOLIDSYSLOG_WINSOCK` are independently auto-detected (POSIX from `<sys/socket.h>`, Winsock from `<winsock2.h>`). On MinGW/Cygwin both can be ON simultaneously. The two example targets (`SolidSyslogExample` and `SolidSyslogWindowsExample` with `OUTPUT_NAME "SolidSyslogExample"`) are intentionally made mutually exclusive via `if(SOLIDSYSLOG_POSIX) ... elseif(SOLIDSYSLOG_WINSOCK AND HAVE_WINDOWS_PLATFORM) ... endif()` to prevent CMake output-name collisions. Do not suggest independent `if` blocks for these two targets in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 137
File: Source/SolidSyslogUdpSender.c:0-0
Timestamp: 2026-04-15T21:12:56.960Z
Learning: In `Source/SolidSyslogUdpSender.c`, NULL checks for `config` and `config->resolver` in `SolidSyslogUdpSender_Create` are intentionally deferred to the error-handling epic (Epic `#31`). The function currently calls `SolidSyslogResolver_Resolve()` dispatch (not the vtable directly) and includes `SolidSyslogResolver.h`. Do not flag missing NULL guards on config/resolver in this function in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:57-71
Timestamp: 2026-04-09T20:59:09.391Z
Learning: In `Source/SolidSyslogTcpSender.c`, error handling for `connect()` failure and `send()` failure (including partial writes, SIGPIPE mitigation, and reconnection logic) is intentionally deferred to story S15.2. In the current implementation (S15.1), `tcp->connected` is set to `true` unconditionally after `connect()`, and `send()` return values are ignored. S15.2 will test-drive: connect failure → return false (and do NOT set `tcp->connected = true`), send failure → close socket and reconnect, and a `SendAll()` helper that retries on partial writes.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 149
File: Platform/Windows/Source/SolidSyslogWinsockResolver.c:67-72
Timestamp: 2026-04-17T20:07:36.721Z
Learning: In `Platform/Windows/Source/SolidSyslogWinsockResolver.c` (and its POSIX mirror `Platform/Posix/Source/SolidSyslogGetAddrInfoResolver.c`), the dereference of `info->ai_addr` without NULL/addrlen validation, and the unchecked cast of `getPort()` to `uint16_t` before `htons()`, are intentional. The resolver trusts its injected `getHost`/`getPort` callbacks by design (SRP); port-range and `ai_addr` defensive checks belong at the config/caller layer. Any future hardening must be a coordinated change across both resolvers together. Do not flag these as bugs or missing validation in future reviews of either resolver file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 91
File: Source/SolidSyslogTcpSender.c:42-50
Timestamp: 2026-04-09T20:59:05.891Z
Learning: In `Source/SolidSyslogTcpSender.c` (and the matching `SolidSyslogUdpSender`), the singleton `Create()` function does not close/clean up a previously opened file descriptor on a repeated call. This is intentional: double-`Create()` is considered a misuse of the singleton pattern, and proper guard/cleanup is deferred to a dedicated error-handling epic. Do not flag this as a bug in future reviews of these files.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 137
File: Interface/SolidSyslogResolverDefinition.h:12-12
Timestamp: 2026-04-15T21:12:31.145Z
Learning: In `Interface/SolidSyslogResolverDefinition.h` (and the corresponding `Source/SolidSyslogResolver.c`), the `Resolve` function pointer returns `void` intentionally. This is a direct extraction of the pre-existing behaviour in `SolidSyslogUdpSender` and `SolidSyslogTcpSender`, neither of which checked resolution success before PR `#137`. Adding a boolean/error return to `Resolve` and propagating failures through sender create/connect paths is deferred to the error-handling epic (Epic `#31`). Do not flag the `void` return on `SolidSyslogResolver.Resolve` as a contract gap in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Source/SolidSyslogFileStore.c:124-128
Timestamp: 2026-04-11T22:49:26.725Z
Learning: In `Source/SolidSyslogFileStore.c`, the return values of `OpenWriteFile()` and `OpenReadFile()` are intentionally unchecked in `SolidSyslogFileStore_Create` (around lines 124–128) and `RotateToNextFile` (around lines 394–400). Guarding against open failures and aborting the create/rotation flow is deferred to the error-handling epic, tracked in `project_robustness_backlog.md` under "FileStore error handling (PR `#103`)". Do not flag missing open-failure guards in these functions in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 140
File: Source/SolidSyslogPosixTcpStream.c:31-36
Timestamp: 2026-04-17T07:50:51.607Z
Learning: In `Source/SolidSyslogPosixTcpStream.c`, `SolidSyslogPosixTcpStream_Destroy()` intentionally does not close the socket fd before nulling the vtable pointers. Through the only supported API path (TcpSender → Stream), every successful `Open` is guaranteed to be matched by a `Close` — either via `TcpSender_Destroy` (when `connected == true`), `SendData` failure recovery, or `Open`'s own connect-failure self-recovery (`OpenClosesSocketOnConnectFailure` test). Standalone "Destroy-without-Close" usage is not a supported scenario; adding defensive cleanup without a driving test would violate the project's "no untested production code" discipline. Formalising this contract (with tests) is deferred to Epic `#31` (error-handling / robustness). Do not flag missing fd cleanup in `SolidSyslogPosixTcpStream_Destroy` in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 131
File: Tests/Support/SafeStringWindows.c:5-8
Timestamp: 2026-04-15T15:53:38.455Z
Learning: In `Tests/Support/SafeStringWindows.c` of `DavidCozens/solid-syslog`, the `SafeString_Copy` Windows implementation intentionally omits a `destSize == 0` guard (unlike `SafeStringStandard.c`). The design goal is to let `strncpy_s` invoke the MSVC invalid parameter handler on misuse, acting as a fail-fast mechanism in test code. Do not flag this asymmetry as a defect in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 109
File: Example/Common/ExampleCommandLine.c:83-88
Timestamp: 2026-04-13T09:52:59.823Z
Learning: In `Example/Common/ExampleCommandLine.c`, the `strtol`-without-endptr/sign-validation pattern for `OPT_MAX_FILES` and `OPT_MAX_FILE_SIZE` (around lines 83–88) is a known pre-existing deferral. The values are clamped downstream in `SolidSyslogFileStore_Create`, so there is no runtime safety issue. Parse-time validation (endptr + non-negative check, returning 1 on bad input) is backlogged for a follow-up PR after PR `#109` merges. Do not re-flag this as a new issue in future reviews of these two `case` branches.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the extern function-pointer seam variables in `Platform/Windows/Source/SolidSyslogWinsockDatagramInternal.h` and `Platform/Windows/Source/SolidSyslogWinsockResolverInternal.h` (`Winsock_socket`, `Winsock_sendto`, `Winsock_closesocket`, `Winsock_getaddrinfo`, `Winsock_freeaddrinfo`) intentionally use underscore-based naming (mirroring the real Winsock API names) to preserve visual clarity at `UT_PTR_SET` call sites. Do not flag these names as non-camelCase in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the `_CRT_SECURE_NO_WARNINGS` compile definition is a known temporary stopgap for MSVC CRT deprecation warnings in test fakes. Issue `#126` (S13.3) will introduce a `SafeString` abstraction (Windows impl wraps `_s` variants; POSIX falls through to standard functions); once test fakes migrate, this suppression is removed. Production code is already SDL-clean. Do not flag this definition as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4200` (flexible array member) is a known stopgap. Issue `#125` (S13.2) will replace `char buffer[]` with the `char buffer[1]` pre-C99 idiom and remove this suppression. Do not flag `/wd4200` in future reviews of this file.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 124
File: CMakeLists.txt:12-14
Timestamp: 2026-04-15T06:15:20.788Z
Learning: In `CMakeLists.txt` of `DavidCozens/solid-syslog`, the MSVC suppression `/wd4297` (exception thrown from `extern "C"` function) is intentional and permanent test-infrastructure design: C fakes require C linkage, and `TestAssert_Fail` throws to propagate failures through CppUTest. Issue `#125` will add an inline comment documenting this justification. Do not flag `/wd4297` as an unjustified suppression in future reviews.
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 103
File: Source/SolidSyslogFileStore.c:174-199
Timestamp: 2026-04-11T21:59:19.899Z
Learning: In `Source/SolidSyslogFileStore.c`, the `ScanForExistingFiles()` function has a known bug: when log files wrap past sequence 99→00, the linear 0–99 scan incorrectly identifies the oldest/newest sequence (numerically first file may actually be the newest). This is a documented deferral in the project notes and will be addressed in a dedicated follow-up story for gap-detection on startup recovery. Do not flag this as a new bug in future reviews of this function.
Learnt from: CR
Repo: DavidCozens/solid-syslog PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-17T21:25:03.776Z
Learning: Applies to Example/Common/**/*.{c,cpp,cc,cxx,h,hpp} : Shared example code (CLI parsing, app name, UDP/TCP config, service thread) goes in `Example/Common/`
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 0
File: :0-0
Timestamp: 2026-04-17T19:45:13.648Z
Learning: In `DavidCozens/solid-syslog`, the test helper methods `Resolve()` and `Result()` in `Tests/SolidSyslogWinsockResolverTest.cpp` intentionally use PascalCase to mirror the convention in the POSIX counterpart `Tests/SolidSyslogGetAddrInfoResolverTest.cpp`. Do not flag PascalCase test helper methods in Winsock or POSIX resolver tests in future reviews.
- Make POSIX and Windows example targets mutually exclusive via elseif. The two targets share the same OUTPUT_NAME (SolidSyslogExample[.exe]), and SOLIDSYSLOG_POSIX + SOLIDSYSLOG_WINSOCK are independent auto-detected options that could both be ON on MinGW/Cygwin, leading to a CMake config error or silent overwrite. - Avoid undefined behaviour on relational compare when one separator pointer is NULL. strrchr can return NULL, and ISO C only defines > between pointers into the same object. Handle NULL branches explicitly before comparing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai re: the nitpick on Summary of dispositions on this review:
|
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 535 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Thanks for the clear summary; glad items ✏️ Learnings added
🧠 Learnings used |
Contributor
☀️ Quality Summary 🚦 Unit Tests (GCC): 99% successful (✔️ 535 passed, 🙈 3 skipped) Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result. |
This was referenced Apr 18, 2026
3 tasks
This was referenced May 4, 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.
Summary
SolidSyslogWindowsExampleCMake target gated onSOLIDSYSLOG_WINSOCK AND HAVE_WINDOWS_PLATFORM. WiresSolidSyslogWinsockResolver+SolidSyslogWinsockDatagram+SolidSyslogWindowsClock/Hostname/ProcessId+WSAStartup/WSACleanupinto a fire-and-forget UDP example. Reuses the POSIX example's interactivesend/quitstdin protocol unchanged.SolidSyslogExample.exe(same base name as the POSIX target) so BDD scenarios stay platform-agnostic — no per-platform forks.ExampleAppName_Setto handle Windows-style\path separators (TDD'd with regression coverage for/, no-separator, and mixed-separator paths).SolidSyslogUdpSender.cout of theSOLIDSYSLOG_POSIX-only group intoSOLIDSYSLOG_POSIX OR SOLIDSYSLOG_WINSOCK— it's platform-agnostic glue and the Windows target needs to link against it.Local end-to-end smoke test
Built with
msvc-debug, fired against the dev-container's syslog-ng on127.0.0.1:5514:Real Windows hostname, real PID, RFC 5424 priority/timestamp/structured-data all present and well-formed.
Known gap deliberately deferred
APP_NAME=SolidSyslogExample.exeincludes the.exesuffix — this will fail the walking-skeleton scenario which expects"SolidSyslogExample". That's a real BDD-driven finding for the next PR (promotesyslog.featureout of@windows_wip). Keeping it out of this PR's scope means each PR stays small and the fix is driven by a failing scenario rather than pre-emptive design.Local gates run
msvc-debugbuild + smoke testdebug(530 tests)clang-debugsanitize(530 tests, ASan+UBSan)cppchecktidycoverageformat(clang-format --Werror)TDD trail
Tests/Example/ExampleAppNameTest.cpp::BackslashSeparatorExtractsBaseName—expected <binary.exe> but was <dir\binary.exe>.ExampleAppName_Setto also consider\and use the rightmost separator./, no-separator, and mixed-separator cases for full branch coverage.Refs #129
Test plan
windows-build-and-testbuilds the newSolidSyslogWindowsExampletarget without errorsbddjob still passes — no behavioural change to the POSIX example or step definitions🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests