New function to redirect the $LOG_FD fd to a changed STDOUT#178
New function to redirect the $LOG_FD fd to a changed STDOUT#178frenck merged 2 commits intohassio-addons:mainfrom
Conversation
WalkthroughA new public function Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant LogLib as bashio::log.reinitialize_output()
participant Shell as Shell FD Table
Caller->>LogLib: call reinitialize_output()
LogLib->>LogLib: is LOG_FD numeric?
alt LOG_FD is numeric
LogLib->>Shell: exec {LOG_FD}>&1 (rebind to current STDOUT)
Shell-->>LogLib: result (success / failure suppressed)
LogLib-->>Caller: return (no error output)
else not numeric
LogLib-->>Caller: no-op (returns)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/log.sh (1)
27-31: Consider adding fd writeability validation for consistency and robustness.The function safely validates that
LOG_FDis numeric before using it in eval, which is correct. However, the validation is less strict than the LOG_FD initialization at line 11, which also checks that the fd is actually writable using{ : >&"${LOG_FD-2}"; }. IfLOG_FDis set to a numeric value for a closed or invalid file descriptor, theexecwill silently fail without indication.While this may be acceptable if callers are expected to verify state, consider matching the validation rigor of line 11 for robustness:
function bashio::log.reinitialize_output() { if [[ "${LOG_FD-}" =~ ^[0-9]+$ ]] && { : >&"${LOG_FD}"; } 2>/dev/null; then eval "exec ${LOG_FD}>&1" fi }This ensures the fd exists and is writable before attempting redirection.
This comment was marked as resolved.
This comment was marked as resolved.
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
|
not stale |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
|
not stale |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
|
not stale |
|
There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days. Thank you for your contributions. |
|
not stale |
There was a problem hiding this comment.
Pull request overview
Adds a helper to realign Bashio logging output when a script changes its STDOUT after LOG_FD has already been established, so subsequent bashio::log.* calls follow the new STDOUT.
Changes:
- Introduces
bashio::log.reinitialize_output()to redirect the existingLOG_FDto the current STDOUT. - Adds documentation/comments describing when add-on developers should call this function.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Proposed Changes
Based on my comment in #174 (#174 (comment))
This is useful in scripts that are not called by S6 and have their STDOUT and STDERR not initialized (ie. redirected to /dev/null) and they have to redirect their STDOUT and STDERR to eg. /proc/1/fd/1. This is valid for eg. Docker healthcheck scripts, some daemons started by S6 services (eg. in case of NetworkManager dispatcher only STDERR has valid value, but crond scripts are started with valid STDOUT and STDERR).
Possible enhancements:
Maybe we can extend it with adding
exec &>/proc/1/fd/1before/outside theif? Add-on scripts can log/write only here. In this case this would be a general reinitialization for scripts not started by S6. This would be a useful addition.Or we can hide this function completely: if the shebang references bashio, bashio checks that STDOUT and STDERR has valid, non /dev/null values, and if any of them is /dev/null, redirects them to /proc/1/fd/1 automatically? This can be a breaking change for some corner cases.
Related Issues
Summary by CodeRabbit