Skip to content

feat: S08.03 slice 4 set command for in-RAM configuration injection#303

Merged
DavidCozens merged 2 commits into
mainfrom
feat/freertos-config-injection
May 9, 2026
Merged

feat: S08.03 slice 4 set command for in-RAM configuration injection#303
DavidCozens merged 2 commits into
mainfrom
feat/freertos-config-injection

Conversation

@DavidCozens

@DavidCozens DavidCozens commented May 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Grows Example/Common/ExampleInteractive with a set NAME VALUE dispatcher parallel to switch and quit — host-TDD'd in 9 ZOMBIES cycles. Library splits args; an integrator-supplied bool (*)(const char* name, const char* value) handler decides accept/reject. Runner echoes set name=value on true, prints set: invalid on false, silently ignores set lines when handler is NULL (mirrors switch).
  • FreeRTOS-side OnSet in Example/FreeRtos/SingleTask/main.c mutates static backing for v1 fields (hostname, appname, procid, msgid, msg, host, port, facility, severity) with RFC 5424 length / range validation. g_message holds facility/severity + pointers into the mutable storage; g_port and g_endpointVersion back the endpoint callbacks (the version bump on set port is what makes UdpSender re-pull and route to the new port).
  • host is plumbed through GetEndpoint but SolidSyslogFreeRtosStaticResolver still uses the hardcoded IPv4 octets — set host … echoes confirmation but doesn't change the destination IP. Follow-up slice teaches the resolver to parse dotted-quads.

Closes #302.

Test plan

  • clang-format --dry-run --Werror on every changed file — clean.
  • ctest --preset debug — 5/5 (ExampleTests 63/63 with 9 new ExampleInteractive tests).
  • cmake --build --preset freertos-cross — both ELFs link clean.
  • QEMU smoke: default hostname on 5514, set hostname QemuFoo mutates next frame, set port 5515 routes to new port, four set: invalid rejections (facility=99, port=99999, empty hostname value, unknown field), clean quit.
  • CI: analyze-tidy, analyze-cppcheck, analyze-iwyu, analyze-format, build-windows-msvc, BDD, OpenSSL integration, coverage.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Introduced interactive set command for dynamic runtime configuration of system fields
    • Supports configuration of hostname, application name, process ID, message ID, message content, host address, port, facility, and severity level
    • Includes comprehensive field validation for string lengths and numeric ranges
    • Automatically increments endpoint version when any configuration value changes

Review Change Stack

Slice 3b.2 hardcoded the FreeRTOS example's identity, message body, and
endpoint into Example/FreeRtos/SingleTask/main.c because the FreeRTOS
port deliberately skips Example/Common/ExampleCommandLine.c (no
getopt). Slice 4 makes those configurable in-RAM via a `set NAME VALUE`
command parallel to `send` and `quit` on the existing UART grammar.

Library-side dispatcher (host-TDD'd, 9 ZOMBIES cycles in
Tests/Example/ExampleInteractiveTest.cpp): ExampleInteractive_Run gains
an ExampleInteractiveSetHandler arg with shape
`bool (*)(const char* name, const char* value)`. The runner splits args
at first whitespace, calls the handler, echoes `set name=value` on true
or prints `set: invalid` on false. NULL handler silently ignores `set`,
mirroring `switch`. POSIX/Threaded/Windows callers pass NULL.

FreeRTOS-side OnSet (integrator glue): mutable static arrays sized to
RFC 5424 maxima, a file-static g_message holding facility/severity in
place plus messageId/msg pointers into the storage, and g_port +
g_endpointVersion behind GetEndpoint/GetEndpointVersion. Validation:
identity / msgid / msg / host non-empty within storage cap; facility
0-23; severity 0-7; port 1-65535. Helpers TryUpdateString and
TryParseUInt do the bounds-check + memcpy + null-terminate and the
full-string strtol parse respectively.

`host` is plumbed through GetEndpoint and accepted into g_host but
SolidSyslogFreeRtosStaticResolver still routes via the hardcoded IPv4
octets — `set host 1.2.3.4` echoes confirmation but doesn't change the
destination IP yet. Follow-up slice teaches the resolver to parse
dotted-quads.

QEMU smoke verifies: default hostname on port 5514, `set hostname Foo`
mutates frames on next send, `set port 5515` routes to the new port
(endpoint-version bump triggers UdpSender re-pull), and four
`set: invalid` rejections (out-of-range, empty, unknown field).

Closes #302

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 9, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@DavidCozens has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 39 minutes and 50 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0d1b738d-ab20-4fd4-9cd7-266e89e0dd8b

📥 Commits

Reviewing files that changed from the base of the PR and between e6d76b7 and d229ef6.

📒 Files selected for processing (3)
  • DEVLOG.md
  • Example/FreeRtos/SingleTask/main.c
  • Tests/Example/ExampleInteractiveTest.cpp
📝 Walkthrough

Walkthrough

This PR adds a set NAME VALUE interactive command for in-RAM configuration mutation in SolidSyslog. The library gains an onSet callback parameter on ExampleInteractive_Run. The FreeRTOS example implements the handler with mutable global state for hostname, appname, procid, msgid, message, host, port, and facility/severity. All four example entry points are updated. Tests validate parser behavior and output messages. A DEVLOG entry documents decisions.

Changes

Set Command for In-RAM Configuration

Layer / File(s) Summary
Public API Contract
Example/Common/ExampleInteractive.h
New ExampleInteractiveSetHandler typedef for bool (*)(const char* name, const char* value) callback; ExampleInteractive_Run signature extended with onSet parameter.
Command Dispatcher
Example/Common/ExampleInteractive.c
HandleSet helper parses name and value from arguments (split at first whitespace), invokes handler, prints set name=value on success or set: invalid on failure. Command dispatch loop routes set input through HandleSet when onSet is non-NULL.
FreeRTOS Configuration State
Example/FreeRtos/SingleTask/main.c
Mutable globals replace TEST_* constants (g_hostname, g_appName, g_processId, g_messageId, g_msg, g_host, g_port, g_endpointVersion). Global g_message struct references the mutable fields. Helper functions TryUpdateString and TryParseUInt provide safe string and numeric updates with validation. OnSet handler validates field names and values (string lengths, numeric ranges for port/facility/severity).
FreeRTOS Callback Integration
Example/FreeRtos/SingleTask/main.c
Formatter callbacks (GetHostname, GetAppName, GetProcessId) read from mutable globals. GetEndpoint populates host and port from globals. GetEndpointVersion returns mutable counter that increments on port change. Interactive runner called with &g_message and OnSet callback.
Call Site Updates
Example/SingleTask/SolidSyslogExample.c, Example/Threaded/main.c, Example/Windows/SolidSyslogWindowsExample.c
All entry points updated to pass fourth NULL argument to ExampleInteractive_Run; FreeRTOS passes OnSet handler.
Parser and Dispatch Tests
Tests/Example/ExampleInteractiveTest.cpp, Tests/Example/CMakeLists.txt
New test file validates set command parsing (name/value extraction, embedded spaces in values), handler invocation with correct arguments, null handler behavior, and stdout output (success echo set name=value, failure message set: invalid). Test utilities include SetHandlerSpy to record invocations.
Development Log
DEVLOG.md
Entry for 2026-05-09 documents S08.03 slice 4 scope, FreeRTOS handler field validation table, endpoint version bumping rationale, QEMU smoke-test scenarios, and pre-PR verification checklist.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • DavidCozens/solid-syslog#297: Main PR builds on the FreeRTOS SingleTask bring-up (PR #297) by modifying the same Example/FreeRtos/SingleTask/main.c to add mutable in-RAM config and an interactive onSet handler.
  • DavidCozens/solid-syslog#301: Related through ExampleInteractive API modifications and integration of the interactive example into the FreeRTOS SingleTask flow.
  • DavidCozens/solid-syslog#153: Touches the ExampleInteractive API and call sites; this PR extends that API with the onSet parameter and updates the same Windows and other example call sites.

Poem

A rabbit hops through mutable config so fine,
set hostname Foo makes the syslog shine,
With validation strong and handlers true,
The FreeRTOS example runs refreshed anew! 🐰

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 6.06% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: implementing a set command for in-RAM configuration injection (S08.03 slice 4), matching the primary objective of the changeset.
Description check ✅ Passed The description provides purpose (closes #302), detailed change summary of the set dispatcher and FreeRTOS handler implementation, test evidence (format check, ctest results, smoke tests), and areas affected. All template sections are substantively covered.
Linked Issues check ✅ Passed All primary objectives are met: ExampleInteractive gains set NAME VALUE with proper handler dispatch [#302], FreeRTOS integrator handler mutates v1 fields with RFC 5424 validation [#302], endpoint version bumping implemented [#302], call sites updated [#302], tests added [#302], and QEMU smoke confirms behavior [#302].
Out of Scope Changes check ✅ Passed All changes align with issue #302 scope: the set dispatcher, FreeRTOS handler integration, tests, and call-site updates are directly required. Flash persistence and resolver dotted-quad parsing are properly deferred per specification.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/freertos-config-injection

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions

github-actions Bot commented May 9, 2026

Copy link
Copy Markdown
Contributor

☀️   Quality Summary

   🚦   build-linux-gcc: 100% successful (✔️ 1105 passed, 🙈 3 skipped)
   🚦   build-linux-clang: 100% successful (✔️ 1042 passed, 🙈 3 skipped)
   🚦   sanitize-linux-gcc: 100% successful (✔️ 1042 passed, 🙈 3 skipped)
   🚦   integration-linux-openssl: 100% successful (✔️ 9 passed)
   🚦   integration-windows-openssl: 100% successful (✔️ 9 passed)
   🚦   bdd-linux-syslog-ng: 100% successful (✔️ 46 passed)
   🚦   bdd-windows-otel: 96% successful (✔️ 44 passed, 🙈 2 skipped)
   🚦   build-windows-msvc: 100% successful (✔️ 954 passed, 🙈 1 skipped)
   ⚠️   Clang-Tidy: 7 warnings (high: 7)
   ⚠️   CPPCheck: No warnings


Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
Example/FreeRtos/SingleTask/main.c (1)

285-299: ⚡ Quick win

Prefer strtoul over strtol for parsing unsigned values.

The function is designed to parse unsigned integers but uses strtol (which parses signed long). While negative values are correctly rejected (line 293), using strtoul would be more semantically correct and would properly handle the full unsigned range and overflow conditions via errno.

♻️ Proposed refactor to use strtoul
 static bool TryParseUInt(const char* value, unsigned long* out)
 {
     if (*value == '\0')
     {
         return false;
     }
-    char* end    = NULL;
-    long  parsed = strtol(value, &end, 10);
-    if ((*end != '\0') || (parsed < 0))
+    char*          end    = NULL;
+    unsigned long  parsed = strtoul(value, &end, 10);
+    if (*end != '\0')
     {
         return false;
     }
     *out = (unsigned long) parsed;
     return true;
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Example/FreeRtos/SingleTask/main.c` around lines 285 - 299, The TryParseUInt
function currently uses strtol for unsigned parsing; replace its use with
strtoul (and include errno.h) so it correctly parses the full unsigned range and
detects overflow via errno==ERANGE; preserve the initial empty-string check,
call strtoul(value, &end, 10), verify end points to '\0', check errno for ERANGE
and treat that as failure, and reject values where end==value (no digits); on
success assign the parsed unsigned long to *out.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@DEVLOG.md`:
- Line 98: Update the unlabeled fenced code block for the QEMU smoke output so
markdownlint MD040 is satisfied by adding a language label (e.g., change the
opening ``` to ```text) for the block that starts with "PORT=5514 BYTES=<134>…
FreeRtosExample … (default)" and ends with "quit (clean exit)"; ensure the
opening fence uses the language token (text) while keeping the existing block
content unchanged.

In `@Example/Common/ExampleInteractive.h`:
- Around line 14-17: The new set callback lacks a void* context parameter;
update the ExampleInteractiveSetHandler typedef to accept a void* context as the
first (or last, consistent with ExampleInteractiveSwitchHandler) parameter and
add a matching void* context argument to the ExampleInteractive_Run signature so
callers can pass instance state; ensure ExampleInteractive_Run forwards that
context when invoking onSet (and keep the existing
ExampleInteractiveSwitchHandler/its usage consistent with the shared context
convention).

In `@Example/FreeRtos/SingleTask/main.c`:
- Around line 235-237: The host-handling branch currently calls
TryUpdateString(g_host, sizeof(g_host), value) but does not increment
g_endpointVersion; update the branch handling strcmp(name, "host") to increment
g_endpointVersion when TryUpdateString returns true (same pattern used for the
"port" branch), and optionally add a short comment referencing
GetEndpointVersion to indicate the version bump is intentional for host changes;
ensure you reference g_host, TryUpdateString, and g_endpointVersion in the
change.

---

Nitpick comments:
In `@Example/FreeRtos/SingleTask/main.c`:
- Around line 285-299: The TryParseUInt function currently uses strtol for
unsigned parsing; replace its use with strtoul (and include errno.h) so it
correctly parses the full unsigned range and detects overflow via errno==ERANGE;
preserve the initial empty-string check, call strtoul(value, &end, 10), verify
end points to '\0', check errno for ERANGE and treat that as failure, and reject
values where end==value (no digits); on success assign the parsed unsigned long
to *out.
🪄 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: 23bcd7a6-b233-4bc3-a28d-b8c255ebf720

📥 Commits

Reviewing files that changed from the base of the PR and between 5b0269a and e6d76b7.

📒 Files selected for processing (9)
  • DEVLOG.md
  • Example/Common/ExampleInteractive.c
  • Example/Common/ExampleInteractive.h
  • Example/FreeRtos/SingleTask/main.c
  • Example/SingleTask/SolidSyslogExample.c
  • Example/Threaded/main.c
  • Example/Windows/SolidSyslogWindowsExample.c
  • Tests/Example/CMakeLists.txt
  • Tests/Example/ExampleInteractiveTest.cpp

Comment thread DEVLOG.md Outdated
Comment on lines +14 to +17
typedef bool (*ExampleInteractiveSetHandler)(const char* name, const char* value);

void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
ExampleInteractiveSetHandler onSet);

@coderabbitai coderabbitai Bot May 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift

Add a context parameter to the new set callback API.

The new callback currently cannot carry per-instance state without globals. Please align this new callback family with the repository callback contract by adding a void* context parameter and a paired context argument in ExampleInteractive_Run.

Proposed API shape
-typedef bool (*ExampleInteractiveSetHandler)(const char* name, const char* value);
+typedef bool (*ExampleInteractiveSetHandler)(void* context, const char* name, const char* value);

-void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
-                            ExampleInteractiveSetHandler onSet);
+void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
+                            ExampleInteractiveSetHandler onSet, void* context);

As per coding guidelines: “For new callbacks, the function pointer must take a void* context parameter; config struct exposes paired context field; when several callbacks form a logical feature, share one context field.”

📝 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
typedef bool (*ExampleInteractiveSetHandler)(const char* name, const char* value);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
ExampleInteractiveSetHandler onSet);
typedef bool (*ExampleInteractiveSetHandler)(void* context, const char* name, const char* value);
void ExampleInteractive_Run(const struct SolidSyslogMessage* message, FILE* input, ExampleInteractiveSwitchHandler onSwitch,
ExampleInteractiveSetHandler onSet, void* context);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Example/Common/ExampleInteractive.h` around lines 14 - 17, The new set
callback lacks a void* context parameter; update the
ExampleInteractiveSetHandler typedef to accept a void* context as the first (or
last, consistent with ExampleInteractiveSwitchHandler) parameter and add a
matching void* context argument to the ExampleInteractive_Run signature so
callers can pass instance state; ensure ExampleInteractive_Run forwards that
context when invoking onSet (and keep the existing
ExampleInteractiveSwitchHandler/its usage consistent with the shared context
convention).

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined for this slice — leaving as-is.

Rationale: ExampleInteractive lives in Example/Common/, which is Tier 3 (best-effort examples, lowest support bar — see CLAUDE.md "Support tiers"). The Callback Conventions section is in scope for library callbacks (Tier 1/2 Core/, Platform/); the existing ExampleInteractiveSwitchHandler next to this one was added without context for the same reason. Adding context to Set now would also pull Switch into scope per the same section's "the class that owns them is next touched" rule (since this PR touches ExampleInteractive), widening the slice beyond config-injection.

The current FreeRTOS OnSet handler uses file-static globals, which is the documented pattern for the example tier — it's already what the existing POSIX ExampleAppName_Set / ExampleSwitchConfig_SetByName helpers do. There's no current consumer for instance-state, and CLAUDE.md "Don't add features beyond what the task requires" / "Don't design for hypothetical future requirements" applies.

If a multi-instance integrator surfaces a real need, we'll migrate Switch + Set together in one principled change rather than bolting context onto one of them now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidCozens, understood — the Tier 3 / YAGNI reasoning is clear and well-documented. I'll withdraw the suggestion and record the exemption for future reviews.


✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 303
File: Example/Common/ExampleInteractive.h:14-17
Timestamp: 2026-05-09T15:56:13.853Z
Learning: In solid-syslog, the CLAUDE.md Callback Conventions section (void* context parameter + paired config field) applies only to Tier 1/2 library callbacks in Core/ and Platform/. Callbacks in Tier 3 example code (Example/Common/, Example/FreeRtos/, etc.) — such as ExampleInteractiveSwitchHandler and ExampleInteractiveSetHandler — are exempt from this requirement. The documented pattern for example-tier handlers is file-static globals. If a multi-instance use-case ever arises, Switch and Set should be migrated together in one principled change rather than piecemeal.

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: 283
File: Platform/OpenSsl/Interface/SolidSyslogTlsStream.h:7-7
Timestamp: 2026-05-07T08:22:53.110Z
Learning: In solid-syslog, the CLAUDE.md callback rule that requires a `void* context` parameter (and the paired config field) should be applied only to callbacks that need to carry real per-instance state. Exempt callbacks that are stateless thin wrappers over OS primitives—i.e., the callback is effectively a pure function of its input parameters with no instance state to preserve—so adding `void* context` would be pointless (the context would always be NULL). For example, platform implementations like `SolidSyslogSleepFunction` that wrap `nanosleep`/`Sleep` and are defined purely in terms of `int milliseconds` should not add `void* context` or the paired config field. For callbacks that do depend on per-instance data, include `void* context` and the paired config field as required by CLAUDE.md.

Comment on lines +235 to +237
if (strcmp(name, "host") == 0)
{
return TryUpdateString(g_host, sizeof(g_host), value);

@coderabbitai coderabbitai Bot May 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Consider bumping g_endpointVersion on successful host changes for forward compatibility.

The PR objectives state "bump GetEndpointVersion on successful port (or host) changes", but this implementation only increments the version for port (line 247), not for host. While the current resolver ignores the host field (per the comment at lines 199–203), bumping the version on host changes would align with the stated objectives and ensure the infrastructure is in place when the resolver is updated in a follow-up slice to actually parse dotted-quads.

If deferring the version bump is intentional (to avoid unnecessary re-resolution while the resolver doesn't use host), consider documenting that decision in a comment or updating the objectives to clarify.

Proposed fix to bump version on host changes
 if (strcmp(name, "host") == 0)
 {
-    return TryUpdateString(g_host, sizeof(g_host), value);
+    if (TryUpdateString(g_host, sizeof(g_host), value))
+    {
+        g_endpointVersion++;
+        return true;
+    }
+    return false;
 }
📝 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 (strcmp(name, "host") == 0)
{
return TryUpdateString(g_host, sizeof(g_host), value);
if (strcmp(name, "host") == 0)
{
if (TryUpdateString(g_host, sizeof(g_host), value))
{
g_endpointVersion++;
return true;
}
return false;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Example/FreeRtos/SingleTask/main.c` around lines 235 - 237, The host-handling
branch currently calls TryUpdateString(g_host, sizeof(g_host), value) but does
not increment g_endpointVersion; update the branch handling strcmp(name, "host")
to increment g_endpointVersion when TryUpdateString returns true (same pattern
used for the "port" branch), and optionally add a short comment referencing
GetEndpointVersion to indicate the version bump is intentional for host changes;
ensure you reference g_host, TryUpdateString, and g_endpointVersion in the
change.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declined — leaving as-is.

Bumping g_endpointVersion on a successful host mutation today would cause SolidSyslogUdpSender to re-pull the endpoint and the resolver to re-resolve, but SolidSyslogFreeRtosStaticResolver still ignores endpoint->host and routes via the Create-time IPv4 octets (see comment in GetEndpoint). That's wasted work without any behavioral change.

The bump lands naturally in the follow-up slice that teaches FreeRtosStaticResolver to parse dotted-quads from g_host (see ## Out of scope in #302"SolidSyslogFreeRtosStaticResolver dotted-quad parsing — separate follow-up so set host actually changes the destination IP"). The DEVLOG entry phrases this as port (or eventually host) — emphasis on eventually — so the deferral is documented.

Aligns with CLAUDE.md "Don't design for hypothetical future requirements".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DavidCozens, understood — the reasoning is sound. Triggering re-resolution while the resolver ignores endpoint->host is pure overhead, and CLAUDE.md's stance against designing for hypothetical future requirements means the bump belongs in the slice that actually wires up dotted-quad parsing. I'll drop this concern.


✏️ Learnings added
Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 303
File: Example/FreeRtos/SingleTask/main.c:235-237
Timestamp: 2026-05-09T15:56:18.170Z
Learning: In DavidCozens/solid-syslog, `g_endpointVersion` is intentionally NOT bumped when `set host` succeeds (in `Example/FreeRtos/SingleTask/main.c`, `OnSet` handler). `SolidSyslogFreeRtosStaticResolver` currently ignores `endpoint->host` and routes via hardcoded `Create`-time IPv4 octets, so bumping the version would cause wasted re-resolution with no behavioral change. The version bump for host changes is deferred to the follow-up slice that teaches the resolver to parse dotted-quads (tracked as out-of-scope in issue `#302`). Do not raise a review finding for the missing version bump on host changes until that follow-up slice lands.

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/Windows/SolidSyslogWindowsExample.c:57-66
Timestamp: 2026-04-18T07:18:55.197Z
Learning: In this project’s example entry-point C files, do not flag/require NULL checks after calls to the solid-syslog `Create` functions (e.g., `SolidSyslogWinsockResolver_Create`, `SolidSyslogWinsockDatagram_Create`, `SolidSyslogUdpSender_Create`, `SolidSyslogNullBuffer_Create`, `SolidSyslogNullStore_Create`, `SolidSyslogAtomicCounter_Create`, `SolidSyslogMetaSd_Create`, `SolidSyslogTimeQualitySd_Create`, `SolidSyslogOriginSd_Create`). These `Create` APIs return pointers to static instances and are designed not to fail at construction time; any runtime failures that can occur are surfaced later via the vtable at use-time (e.g., host resolution). Therefore, NULL checks after `Create` calls in example files (such as `Example/Windows/SolidSyslogWindowsExample.c` and similar examples) are intentionally omitted and should not be treated as a bug.

Learnt from: DavidCozens
Repo: DavidCozens/solid-syslog PR: 291
File: Example/FreeRtos/HelloWorld/CmsdkUart.c:17-42
Timestamp: 2026-05-08T17:19:38.382Z
Learning: In this codebase (DavidCozens/solid-syslog), internal driver/config/access structs may intentionally store raw pointers without NULL checks, `isInitialized` flags, or defensive struct copies. Treat this as an explicit project policy: do not flag store-by-pointer usage or missing NULL/initialization guards for these internal config/access structs when the pointer is provided by production callers and is guaranteed to point to `static const` objects with program lifetime (per CLAUDE.md). Only flag NULL/initialization issues when the pointer originates from potentially invalid/external/untrusted sources or when the code does not establish lifetime/guarantees.

@DavidCozens

Copy link
Copy Markdown
Owner Author

Review summary — fixes incoming + decisions

Accepted (applied in next commit):

  • DEVLOG MD040 fence label (#3213368108).
  • strtoul over strtol in TryParseUInt (review-body nitpick) — behaviour preserved because each call site already range-checks (port ≤ UINT16_MAX, facility ≤ 23, severity ≤ 7), and strtoul's wrap-on-negative is caught by those upper bounds.

Also fixed (CI surfaced these alongside the CodeRabbit findings):

  • cppcheck [cstyleCast] at Tests/Example/ExampleInteractiveTest.cpp:65(void*) inputconst_cast<char*>(input) with rationale comment (fmemopen mode "r" doesn't write).
  • clang-tidy cppcoreguidelines-pro-type-member-init at SetHandlerSpy — added in-class default initializers for callCount / returnValue.
  • clang-tidy cppcoreguidelines-owning-memory ×5 (dup, freopen, fopen, fclose ×2) — // NOLINTNEXTLINE per the established pattern in Tests/Example/SolidSyslogExampleTest.cpp (test infrastructure; no ownership transfer).
  • clang-tidy clang-analyzer-unix.Stream at the fread loop — // NOLINTNEXTLINE (fread on EOF/error returns 0 and exits the loop cleanly).
  • clang-tidy cppcoreguidelines-pro-type-const-cast on the const_cast introduced for the cppcheck fix above — // NOLINTNEXTLINE cross-referencing the existing rationale comment.

Declined (rationale on each thread):

  • Add void* context to ExampleInteractiveSetHandler (#3213368110) — Tier 3 example code; would force a parallel Switch migration and widen the slice. See thread reply.
  • Bump g_endpointVersion on host mutation (#3213368115) — wasted work today (resolver still ignores endpoint->host); the bump lands in the follow-up slice that wires the resolver to the host string. See thread reply.

Local verification before pushing the fix-up commit: cmake --build --preset cppcheck clean, cmake --build --preset tidy clean, cmake --build --preset freertos-cross clean, ExampleTests 63/63 (77 checks).

cppcheck (`[cstyleCast]`): replace `(void*) input` in
ExampleInteractiveTest's RunWithInput with `const_cast<char*>(input)`
+ rationale comment (fmemopen mode "r" never writes to the buffer).

clang-tidy (analyze-tidy):
- SetHandlerSpy: in-class default initializers for callCount and
  returnValue (cppcoreguidelines-pro-type-member-init).
- Stdout-capture helpers + RunWithInput: NOLINT comments for the
  owning-memory checks on dup / freopen / fopen / fclose ×2 and the
  clang-analyzer-unix.Stream check on the fread loop, matching the
  pattern already established in SolidSyslogExampleTest.cpp for the
  same POSIX-C test-helper idioms.
- const_cast in RunWithInput: NOLINT for
  cppcoreguidelines-pro-type-const-cast cross-referencing the
  existing rationale comment.

CodeRabbit nitpick (TryParseUInt): switch from `strtol` + explicit
`< 0` rejection to `strtoul` for semantic correctness on unsigned
parsing. Behaviour preserved because each call site already
range-checks (port <= UINT16_MAX, facility <= 23, severity <= 7) and
strtoul's wrap-on-negative is caught by those upper bounds; comment
documents the contract.

CodeRabbit MD040: add `text` language tag to the QEMU smoke fenced
block in the DEVLOG slice-4 entry.

Declined (replies on threads, no code change):
- ExampleInteractiveSetHandler context parameter — Tier 3 example
  code; would also pull Switch migration into scope per CLAUDE.md
  "the class that owns them is next touched".
- g_endpointVersion bump on host mutation — wasted work today
  (resolver still ignores endpoint->host); lands in the follow-up
  slice that wires the resolver to dotted-quad parsing.

Local verification: cppcheck preset clean, tidy preset clean,
freertos-cross builds clean, ExampleTests 63/63 (77 checks),
clang-format --dry-run --Werror clean on all touched files.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented May 9, 2026

Copy link
Copy Markdown
Contributor

☀️   Quality Summary

   🚦   build-linux-gcc: 100% successful (✔️ 1105 passed, 🙈 3 skipped)
   🚦   build-linux-clang: 100% successful (✔️ 1042 passed, 🙈 3 skipped)
   🚦   sanitize-linux-gcc: 100% successful (✔️ 1042 passed, 🙈 3 skipped)
   🚦   integration-linux-openssl: 100% successful (✔️ 9 passed)
   🚦   integration-windows-openssl: 100% successful (✔️ 9 passed)
   🚦   bdd-linux-syslog-ng: 100% successful (✔️ 46 passed)
   🚦   bdd-windows-otel: 96% successful (✔️ 44 passed, 🙈 2 skipped)
   🚦   build-windows-msvc: 100% successful (✔️ 954 passed, 🙈 1 skipped)
   ⚠️   Clang-Tidy: No warnings
   ⚠️   CPPCheck: No warnings


Created by Quality Monitor v1.14.0 (#f3859fd). More details are shown in the GitHub Checks Result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S08.03 slice 4: set command for in-RAM configuration injection

1 participant