-
-
Notifications
You must be signed in to change notification settings - Fork 20
Allow child bashio processes to access the parent LOG_FD variable #174
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughUpdates Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Shell as User Shell
participant Script as lib/log.sh
participant Env as Environment
participant Child as Child Process
participant STDOUT as Original STDOUT (preserved FD)
Shell->>Script: source/run script
Script->>Script: test/expand LOG_FD using ${LOG_FD:-}
Script->>STDOUT: duplicate original STDOUT onto free fd (LOG_FD)
Script->>Env: export LOG_FD
Note over Script,Env: exported for inheritance
Shell->>Child: spawn subprocess
Child->>Env: inherits LOG_FD
Child->>STDOUT: write logs via inherited LOG_FD
Note over Child,STDOUT: logs routed through preserved FD
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Potential focus areas:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/log.sh(1 hunks)
🔇 Additional comments (2)
lib/log.sh (2)
11-11: Robust fd validation check — good guard.Using ${LOG_FD:-} with a writability probe keeps set -u safe and avoids inheriting a stale/closed fd. LGTM.
13-14: Comment clarity improved.The note now accurately distinguishes subshell vs child process behavior. Thanks.
Can you elaborate what exact real world use-case that fixes? Do you have an example from an add-on where that is actually necessary? From my testing, the current approach is enough to fix function calling, which from all I know is really used in real world (see #172 (comment)). |
sairon
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agners while I can't give a real-world example from the wild, I think I get what @PaulSD means. Here's a reproducer:
$ cat test.sh
#!lib/bashio
echo "PID: $$"
echo "LOG_FD: $LOG_FD"
bashio::log.info "Hello from $$, LOG_FD: $LOG_FD"
./subprocess.sh > test.out
$ cat subprocess.sh
#!lib/bashio
echo "PID: $$"
echo "LOG_FD: $LOG_FD"
bashio::log.info "Hello from $$, LOG_FD: $LOG_FD"
if bashio::fs.file_exists $0; then
echo "I exist"
fi
./subprocess2.sh
$ cat subprocess2.sh
#!lib/bashio
echo "PID: $$"
echo "LOG_FD: $LOG_FD"
bashio::log.info "Hello from $$, LOG_FD: $LOG_FD"Without the change:
$ LOG_LEVEL=7 ./test.sh
PID: 79493
LOG_FD: 10
[12:08:38] INFO: Hello from 79493, LOG_FD: 10
$ cat test.out
PID: 79498
LOG_FD: 11
[12:08:38] INFO: Hello from 79498, LOG_FD: 11
[12:08:38] TRACE: bashio::fs.file_exists: ./subprocess.sh
I exist
PID: 79504
LOG_FD: 12
[12:08:38] INFO: Hello from 79504, LOG_FD: 12
With the change:
$ LOG_LEVEL=7 ./test.sh
PID: 80223
LOG_FD: 10
[12:09:43] INFO: Hello from 80223, LOG_FD: 10
[12:09:43] INFO: Hello from 80228, LOG_FD: 10
[12:09:43] TRACE: bashio::fs.file_exists: ./subprocess.sh
[12:09:43] INFO: Hello from 80234, LOG_FD: 10
$ cat test.out
PID: 80228
LOG_FD: 10
I exist
PID: 80234
LOG_FD: 10
IMO the latter is the correct behavior, as there's indeed no mixing of the logging and stdout. Also there's difference in the behavior when the LOG_FD is set for the parent process - as the guard is applied then for all subprocesses and it essentially behaves the same way as the fixed version. While you could argue that you can explicitly export LOG_FD before you call the first bashio script from the main one, I don't think it's really intuitive.
| # Export LOG_FD for use by child bashio processes. | ||
| export LOG_FD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can move this to the conditional block above 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually did that on my first attempt, but CodeRabbit suggested making it unconditional, and the justification CodeRabbit provided makes sense (making it unconditional potentially avoids issues in an unlikely corner case), so I changed it to unconditional. See: #174 (comment)
I've added a comment to explain why it is unconditional.
Sure, I understand how it could come to this situation. But do we need to support this case 🤔 🤷 ? The move to stderr changed the contract of how any bashio script can be used, this is even true with all this changes, e.g. if the stdout of the "root" script is parsed somewhere, that will behave differently as well: LOG_LEVEL=7 ./test.sh | grep HelloI'd make the point why does the root bashio script should be different than subscripts?
We started off to redirect logging to stdout, since stderr felt wrong. Now we kinda separate it into yet another fd, but only for sub scripts. IMHO, we made the conscious decision that basio scripts should log to stdout, and we should stick with that. Unfortunately it also broke internal function calling. But that is fixed by #172. But no more magic needed, it's now logging to stdout. |
|
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. |
|
@agners
Scanning through a few add-ons that I am familiar with, I don't see any examples where the add-on is broken out-of-the-box and requires this PR to fix it. However, I think it is entirely plausible that a developer or user could set up an add-on in a way that breaks things without this PR. As a slightly contrived but plausible example: As-is, setting In addition to that example, note that you seemed to agree that this fix was worthwhile in this comment, where you said:
|
|
Note that #177 (comment) mentions running bashio scripts from other bashio scripts, which is exactly the use case that this PR tries to help with. |
TLDR: If an add-on, that has some custom bashio script (in /usr/bin), and the output of this script is captured anywhere, that add-on will crash if the log level is set to debug. I've added I'm really surprised, that after bashio v0.17.2 (2 month ago), nobody run into this problem. |
|
And I've found another issue. In case of healthcheck or eg. networkmanager scripts (NM dispatcher is started as an s6 service), when bashio's After &1 and &2 is "initialized" in the script, a new bashio::log.??? function would be useful (sg. like bashio::log.reinitialize_output), with sg. like this (note: eval is needed, otherwise 10 not found is the result): Just for reference:
So a |
Right, I came around on the LOG_FD variable thing: Knowing that bash optimized these function calls to run in the same process, which had the side effect making our logging work as we intend. But it's safer to make sure even a sub-process gets the logging fd. |
|
FYI: Created a new PR #178 to handle the STDOUT reinitialization case (mentioned in my comment #174 (comment)) |
This fixes another potential problem with #169 which #172 failed to fix.
Specifically (as described in #172 comment), log messages and stdout could still get mixed up if one bashio script calls another bashio script as a child (not as a subshell).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation