Log WireGuard command errors to syslog - #2401
Conversation
PR Summary by QodoLog WireGuard command failures to syslog
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1.
|
|
Tested on a real camera with BusyBox 1.36.1. A missing required U-Boot environment variable causes |
|
For human reviewers: One more thought: some of the explicit error descriptions may not be necessary. The syslog entry already includes the command that failed, which often makes it clear what operation could not be completed. The additional message is useful where it adds context, but perhaps not every command needs one. |
openipc-ai
left a comment
There was a problem hiding this comment.
Thanks for this — the syslog capture is the right idea, and the local output / local rc=$? split is correct (checked under busybox ash: rc really does get the command's status, which is the trap most versions of this helper fall into). Reading the environment before modprobe/ip link add is a real improvement too: master creates and configures wg0 and only then exits on a missing wg_address, leaving a half-built interface behind.
Qodo's kill -s TERM $$ finding is genuinely fixed in c6e4104 — I checked the commit history rather than trusting the resolved marker.
Three things to fix before this lands, left inline below. Two of them are reachable on a stock configuration, and this file is in general/overlay/, so it ships to all 99 boards (ci-matrix.py --stdin widens to the full matrix).
Evidence
CLAUDE.md asks a behaviour-changing PR for pasted before/after output rather than a description of it, and here the output is the feature. Could you add the actual logread lines from one of the four boards — a failing run showing the three lines run_cmd writes? The link to #2319 covers the reproduction, but not what the new logging looks like on a camera.
Separately, for a maintainer: the five workflows on this PR are all sitting at action_required, so nothing has built yet.
| printf '%s\n' "$output" | | ||
| while IFS= read -r line; do | ||
| logger -p user.err -t "$prog[$$]" "$line" | ||
| done |
There was a problem hiding this comment.
$output is empty on the most common failure here, and printf '%s\n' "" still feeds one empty line to logger.
This tree patches fw_printenv so that -n suppresses ## Error: "x" not defined (general/package/all-patches/uboot-tools/0011-env-partition-autosearch.patch), so an unset variable exits 1 with nothing on stderr. Each of the six environment reads below therefore logs a trailing blank line:
Error: Failed to read wg_alive U-Boot environment variable.
Command `fw_printenv -n wg_alive` finished with code 1:
<blank>
Wrapping the loop in [ -n "$output" ] drops it.
| if [ -n "$var" ]; then | ||
| eval "$var=\"\$output\"" | ||
| else | ||
| printf '%s\n' "$output" | ||
| fi |
There was a problem hiding this comment.
This printf runs on success too, so every run_cmd call with an empty var writes a newline to stdout. Byte-exact stdout of a fully successful run (five void commands plus one per wg_allowed entry):
this PR: 0000000 \n \n \n \n \n \n \n
master: 0000000
That breaks the WebUI. majestic-webui's www/cgi-bin/wireguard.cgi runs this script synchronously and unredirected inside a haserl <% %> block, and the next thing it calls is redirect_to, which writes the raw status line itself (p/common.cgi:517):
/usr/sbin/wireguard
sleep 1
redirect_to "$SCRIPT_NAME" "success" "WireGuard is up"The blank lines land ahead of HTTP/1.1 303 See Other, so the "WireGuard is up" redirect stops working after a save.
Nothing reads run_cmd's stdout any more now that every value-returning call passes var, so the simplest fix is to drop the else branch and keep only the assignment.
| run_cmd "" "" ip address add dev wg0 "$WG_ADDRESS" | ||
| run_cmd "" "" ip link set up dev wg0 | ||
|
|
||
| for i in $(echo "$WG_ALLOWED" | tr ',' ' '); do | ||
| run_cmd "" "" ip -4 route add "$i" dev wg0 | ||
| done |
There was a problem hiding this comment.
Making these fatal is a regression against #1897, which added the route loop with per-entry failures deliberately tolerated.
Both failure modes are reachable on a stock camera — checked against real iproute2:
ip -4 route add ::/0 dev wg0 -> rc=1 Error: inet prefix is expected rather than "::/0".
ip -4 route add 0.0.0.0/0 dev wg0 -> rc=2 RTNETLINK answers: File exists (eth0 default route already present)
So with wg_allowed=0.0.0.0/0,::/0,10.9.0.0/24:
- master installs
0.0.0.0/0, ignores the::/0failure, installs10.9.0.0/24 - this version installs
0.0.0.0/0, exits on::/0, and never installs10.9.0.0/24
S98wireguard backgrounds the script, so the non-zero exit is invisible. What shows up on the camera is a missing route and a tunnel that half works — exactly the kind of failure this PR is trying to make visible. ip address add has the same shape on a re-run, where it fails with File exists and now takes ip link set up down with it.
Suggest keeping these three non-fatal — log and carry on — and reserving exit for the steps that genuinely cannot be survived.
|
Why do you recommend making |
openipc-ai
left a comment
There was a problem hiding this comment.
Why do you recommend making
ip address add,ip link set up, and route creation non-fatal? If any of these commands fails, the WireGuard tunnel is not fully operational. Wouldn't treating them as fatal be more appropriate?
Because exiting there doesn't prevent a broken tunnel — it builds a more broken one. By that point wg0 exists, carries the peer config, and (in the route loop) already has its address and is up. exit rolls none of that back; it only skips the work that is left. Failing on entry 2 of 3 leaves entry 1 installed and entry 3 missing, which is strictly worse than installing 1 and 3 and logging 2.
And the failures that dominate here are not breakage. RTNETLINK answers: File exists means the route or address is already present — that is the desired end state, and it is what a re-run produces; the WebUI runs /usr/sbin/wireguard on every save. An IPv6 entry in wg_allowed is valid WireGuard configuration too — it belongs in AllowedIPs, it just is not an IPv4 route — so ip -4 route add ::/0 failing is the script misreading a correct config as an error. wg_allowed is a list; "operational" is not one bit, and one rejected entry should not cost the others.
There is also an argument specific to this PR: the usual reason to abort early is that nobody would otherwise notice the failure. Your change removes that. Once it is in syslog, doing as much as possible and logging the rest beats stopping.
Where I agree with you: fatal is right when a failure makes everything after it meaningless — modprobe, ip link add, wg setconf — and those should stay fatal. ip address add is the genuinely arguable one, since a tunnel with no address is dead. If you would rather keep it fatal, tolerate EEXIST first, otherwise the re-run path kills it.
On 8d912d1: all three earlier findings are fixed — checked under busybox ash, all three routes now install where the previous revision stopped at ::/0, and both test_shell_parse.sh and test_strip_shell_comments.sh still pass. Three new things came in with the rework, inline below.
For a maintainer: the five workflow runs on c6e4104 all finished green, including 100/100 board build jobs. The runs on 8d912d1 are back at action_required; probably worth approving them after this round rather than spending the matrix twice.
| if [ -n "$msg" ]; then | ||
| logger -p user.err -t "$prog[$$]" "$msg" | ||
| elif [ -z "$output" ]; then | ||
| logger -p user.err -t "$prog[$$]" "Command \`$*\` finished with code $rc." | ||
| else | ||
| logger -p user.err -t "$prog[$$]" "Command \`$*\` finished with code $rc:" | ||
|
|
||
| printf '%s\n' "$output" | | ||
| while IFS= read -r line; do | ||
| logger -p user.err -t "$prog[$$]" "$line" | ||
| done | ||
| fi |
There was a problem hiding this comment.
This branch is now either/or, so setting msg suppresses the command's own output in syslog — which undoes what the PR set out to do. Same failing wg setconf, both revisions:
8d912d15: Failed to apply wireguard configuration
c6e4104a: Error: Failed to apply wireguard configuration.
Command `wg setconf wg0 /tmp/wireguard.conf` finished with code 1:
Line unrecognized: `PersistentKeepalive='
Configuration parsing error
Line unrecognized / Configuration parsing error is the diagnostic #2319 was chasing — it is the difference between "WireGuard did not start" and "PersistentKeepalive is empty". It still reaches stderr via the block below, but S98wireguard backgrounds this script at boot, so stderr goes to the console: the thing the PR description opens by calling easy to miss when the camera is remote.
Suggest logging the message and the output — msg as the headline, then the command line and its output as before.
|
|
||
| if [ "$fatal" = "true" ]; then | ||
| exit "$rc" | ||
| fi |
There was a problem hiding this comment.
Trailing tab after fi.
| if [ -n "$var" ]; then | ||
| eval "$var=\"\$output\"" | ||
| elif [ -n "$output" ]; then | ||
| echo "$output" | ||
| fi |
There was a problem hiding this comment.
--non-fatal clears var at the top, so after a non-fatal failure execution falls through to here and prints the error a second time — once to stderr at line 37, once to stdout here.
The stdout copy is the WebUI problem from my earlier comment, still live. Byte-exact stdout of a run where one route fails:
0000000 E r r o r : i n e t p r e f i x ...
A first save through the WebUI on a camera that already has a default route hits RTNETLINK answers: File exists on 0.0.0.0/0, and that text lands in the CGI body ahead of redirect_to's HTTP/1.1 303 See Other.
Since the failure path already writes to stderr and syslog, this branch only needs to run when rc is 0.
Two smaller notes on the same block: --non-fatal occupying the var slot means a command can never be both non-fatal and value-returning, which will bite when this helper moves to a shared library; and echo "$output" is worth keeping as printf '%s\n' "$output", since busybox echo can eat backslashes in some builds.
Read the required WireGuard U-Boot environment variables before waiting for DNS. This makes missing configuration fail immediately instead of unnecessarily waiting for the endpoint hostname to resolve. Use descriptive shell variable names corresponding to the U-Boot environment variables and include their names in error messages.
a2fdf97 to
ac915ea
Compare
openipc-ai
left a comment
There was a problem hiding this comment.
All findings addressed, and the rework is cleaner than what I suggested — folding the default message into msg keeps the message and the command output travelling together, and the Command ... finished with code N line carries the exit status either way.
Re-verified on the rebased head under busybox ash: stdout stays empty on a non-fatal route failure so the WebUI's redirect_to is unaffected, all three entries of a mixed v4/v6 wg_allowed are attempted, and a missing variable logs two clean lines with no stray blank. test_shell_parse.sh, test_strip_shell_comments.sh and ci-matrix.py --self-test all pass.
CI is green on ac915ea: 114 checks, 100/100 board builds, including hi3519v101_lite now that #2404 has landed.
Rebased onto master for you — #2404 touched only hi3519v101.generic.config, so nothing conflicted and the diff is unchanged.
Problem
WireGuard initialization errors are currently printed to the console only, which makes them easy to miss when the camera is accessed remotely.
Solution
Add a
run_cmd()helper that captures command output and exit codes. When a command fails, the error, exit code, and command output are written to syslog before the script terminates with the original exit code.This helper could be moved to a common shell library in the future and reused by multiple OpenIPC scripts. The global variable names would need to be adjusted first to avoid conflicts with variables in the calling scripts.
Hardware tested on
The fix has been tested on these platforms.
Evidence
See OpenIPC/firmware#2319 for the problem reproduction, testing, and discussion.
Scope
general/package/all-patches/linux/(those go to https://github.com/OpenIPC/linux)general/overlay/or in a sharedload_<vendor>script hardcodes a value specific to my boardLD_PRELOAD, and no binaries that cannot be rebuilt from source