Skip to content

variants/linux: validate meshcored.ini, and stop hiding failures at startup - #2

Open
mmmorks wants to merge 1 commit into
pr/01-build-toolingfrom
pr/02-config-validation
Open

variants/linux: validate meshcored.ini, and stop hiding failures at startup#2
mmmorks wants to merge 1 commit into
pr/01-build-toolingfrom
pr/02-config-validation

Conversation

@mmmorks

@mmmorks mmmorks commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Staged on the fork for review. Final destination: l5yth/meshcore-linux linux. Base here is pr/01-build-tooling so the diff shows only this change.

Summary

The INI loader parsed every value with atoi()/atof() and silently dropped anything it did not understand, so a misconfigured node started up looking healthy and failed later in a way that read as a hardware fault. This makes every problem visible at startup and refuses to start on the ones that cannot be honoured.

What changed

  • LinuxConfig::load() returns a LoadResult that separates two failure kinds, because they deserve opposite responses:
    • An invalid value is fatal. dio2_as_rf_switch = true used to become false (leaving TX dead on boards that need the RF switch); lora_irq_pin = 260 wrapped to line 4 through pin_size_t; lora_freq = 868,5 became 868.0. There is no sensible fallback for "which GPIO drives the radio", so the daemon now says which line is wrong and exits.
    • An unknown key only warns. It is inert by definition and may come from a newer or older build, so refusing to boot over it would take a working repeater off-air for a line it was already ignoring. It is still said out loud, because the radio parameters are first-run defaults that get persisted on the first boot: lora_frequency for lora_freq did not merely fail to apply, correcting the INI afterwards did not undo it.
    • A file that cannot be opened at all is now distinguishable from one that was read.
  • Parsing: booleans accept 1/0, true/false, on/off, yes/no; pins are range-checked against 0..255; integers against their field's width; floats must parse to the end of the value. Also handled: an overlong line (which fgets() used to split into a truncated setting plus a bogus unknown key), a UTF-8 BOM gluing itself to the first key, [section] headers, a line with no =, and a duplicate key (which leaked the previous safe_copy() allocation). The string fields become const char* since they may point at literals.
  • reboot() re-execs the process via the ardulinux core's ::reboot() instead of exit(0). The shipped unit uses Restart=on-failure, so a clean exit is a stop, not a restart, and reboot/clkreboot reach this from a remote admin over the mesh, which took an unattended node off-air.
  • LinuxRTCClock::setCurrentTime() reports what happened. Setting the host clock needs CAP_SYS_TIME, which the unprivileged unit does not have, so clock sync and GPS time sync were reporting success and doing nothing. A failure is now warned about once; a successful set is logged every time so a clock fighting NTP is visible in the journal.
  • The placeholder Module built at static init (before the pins are known) is freed when radio_init() rebuilds the radio, rather than leaked.
  • startup_reason is set to BD_STARTUP_NORMAL; nothing initialised it before.
  • README: a "Config validation" subsection and the accepted boolean spellings.

How it was tested

  • The loader was run in an arm64 container against 20 crafted config files, with a harness that checks each case in both directions (whether startup reached the SPI stage), so a warning that silently became fatal fails and so does a fatal that silently became a warning; all three shipped templates still validate. radio_init() was confirmed to reach the delete/reassign path in the same container.
  • On this branch as submitted: arm64 build in Docker succeeds; pio test -e native passes (no tests are added here; the loader has no host-side harness in the tree).
  • reboot() and the clock-set diagnostics follow from the shipped unit (Restart=on-failure, unprivileged user, NoNewPrivileges=yes); those two changes were verified by build, not by a dedicated test.

Dependencies

Based on the build-tooling PR (only so the native suite compiles on macOS); no code dependency.

Shared code touched

None (all under variants/linux/).

…tartup

The INI loader parsed every value with atoi()/atof() and dropped anything it
did not understand on the floor, so a misconfigured node started up looking
healthy and failed later in a way that read as a hardware fault:

  * `dio2_as_rf_switch = true` (the spelling every other bool invites) became
    false, leaving the RF switch unset and TX dead on boards that need it.
  * `lora_irq_pin = 260` wrapped to line 4 through pin_size_t and failed at RX.
  * `lora_freq = 868,5` became 868.0; `lora_freq = abc` became 0.0.
  * `lora_frequency` (for `lora_freq`) was silently ignored -- and because the
    radio parameters are first-run defaults that MyMesh::begin() persists to
    prefs.json on the first boot, correcting the INI afterwards did not undo it.
  * A file that could not be opened at all was indistinguishable from one that
    was read.

load() now returns a LoadResult that separates the two failure kinds, because
they deserve opposite responses. An invalid value is fatal: the operator wrote
a specific setting and it cannot be honoured, and there is no sensible fallback
for "which GPIO drives the radio". An unknown key only warns: it is inert by
definition, may come from a newer or older build, and refusing to boot over it
would take a working repeater off-air for a line it was already ignoring. Both
are reported on their own ERROR: line naming the key and value. Booleans accept
1/0, true/false, on/off, yes/no; pins are range-checked against 0..255; ints
against their field's width; floats must parse to the end of the value. Also
handled: an overlong line (fgets() would otherwise split it into a truncated
setting plus a bogus unknown key), a UTF-8 BOM gluing itself to the first key,
`[section]` headers, a line with no '=', and a duplicate key (which used to
leak the previous safe_copy() allocation; assign_string() tracks whether the
field still points at its literal default). The string fields become
const char*, since they may point at literals.

Three smaller things in the same spirit:

  * reboot() re-execs the process via the ardulinux core's ::reboot() instead
    of exit(0). The shipped unit uses Restart=on-failure, so a clean exit is a
    stop, not a restart -- and `reboot`/`clkreboot` reach this from a remote
    admin over the mesh, which took an unattended node off-air.
  * LinuxRTCClock::setCurrentTime() reports what happened. Setting the host
    clock needs CAP_SYS_TIME, which the unprivileged unit does not have, so
    `clock sync` and GPS time sync were reporting success and doing nothing.
    A failure is now warned about once; a successful set is logged every time,
    so a clock fighting NTP is visible in the journal.
  * The placeholder Module built at static init (before the pins are known)
    is freed when radio_init() rebuilds the radio, rather than leaked.

startup_reason is set to BD_STARTUP_NORMAL, which nothing did before.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EXSCjgNEbJfHwLjD2WSHW4
@mmmorks
mmmorks force-pushed the pr/02-config-validation branch from b61c48c to 037b93f Compare September 8, 2026 04:11
@mmmorks
mmmorks force-pushed the pr/01-build-tooling branch from bb47913 to 41a35c8 Compare September 8, 2026 04:11
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.

1 participant