Redirect LOG_FD also when suppressing log messages#669
Conversation
WalkthroughThree bash scripts in the Tailscale addon are updated to wrap supervisor API calls with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tailscale/rootfs/usr/bin/protect-subnet-routes (1)
29-30: Consider a shared silent Supervisor probe helper.This exact
eval+ dynamicLOG_FDredirection now appears twice in this file and in both s6runscripts. A small wrapper would give you one place to validateLOG_FDand one place to adjust the redirection syntax later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tailscale/rootfs/usr/bin/protect-subnet-routes` around lines 29 - 30, Extract the duplicated silent Supervisor probe logic (the eval "bashio::api.supervisor GET ... &> /dev/null ${LOG_FD}> /dev/null" pattern) into a single helper function (e.g., supervisor_silent_probe or probe_supervisor) that validates/normalizes LOG_FD and performs the dynamic redirection, then replace both occurrences (the while loop condition in protect-subnet-routes and the two s6 run scripts) with calls to that helper; ensure the helper accepts the full API path string and returns the original command exit status so existing while/if logic (using the helper name) works unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/run`:
- Line 79: LOG_FD is interpolated directly into eval which breaks when it
contains non-digits; add the same guard used in healthcheck/protect-subnets to
validate LOG_FD is numeric before embedding it. Specifically, in forwarding/run
(the eval invoking "bashio::api.supervisor GET /network/interface/default/info
..."), and likewise in protect-subnet-routes and local-network/run, check that
LOG_FD matches a digits-only regex (e.g. ^[0-9]+$) and only append the
redirection token into the eval when that check passes (otherwise fall back to a
safe default or omit the redirection). Reference the existing validation pattern
in healthcheck/protect-subnets and apply it to these eval usages so non-numeric
inherited values do not produce shell syntax errors.
---
Nitpick comments:
In `@tailscale/rootfs/usr/bin/protect-subnet-routes`:
- Around line 29-30: Extract the duplicated silent Supervisor probe logic (the
eval "bashio::api.supervisor GET ... &> /dev/null ${LOG_FD}> /dev/null" pattern)
into a single helper function (e.g., supervisor_silent_probe or
probe_supervisor) that validates/normalizes LOG_FD and performs the dynamic
redirection, then replace both occurrences (the while loop condition in
protect-subnet-routes and the two s6 run scripts) with calls to that helper;
ensure the helper accepts the full API path string and returns the original
command exit status so existing while/if logic (using the helper name) works
unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8b0276c-2761-423a-b7b4-98a7d3c307ae
📒 Files selected for processing (3)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/forwarding/runtailscale/rootfs/etc/s6-overlay/s6-rc.d/local-network/runtailscale/rootfs/usr/bin/protect-subnet-routes
Proposed Changes
Sadly we must use eval,
${LOG_FD}> /dev/nullnever redirected, only string constants (like10> /dev/nullare interpreted as file descriptor. This is how bash works.Related Issues
Summary by CodeRabbit