-
Notifications
You must be signed in to change notification settings - Fork 1.8k
util: Redirect stdin, stdout, and stderr to /dev/null in case of demonize
#11219
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: master
Are you sure you want to change the base?
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughDaemonization in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/flb_utils.c
Outdated
| /* Redirect stdin, stdout, stderr to `/dev/null`. */ | ||
| fd = open("/dev/null", O_RDWR); | ||
| if (fd != -1) { | ||
| dup2(fd, STDIN_FILENO); |
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.
Include fcntl for daemon fd redirection
In flb_utils_set_daemon (src/flb_utils.c), the new redirection block calls open("/dev/null", O_RDWR) and relies on O_RDWR, but this compilation unit does not include <fcntl.h>. On POSIX builds with FLB_HAVE_FORK enabled, O_RDWR and the declaration of open are only provided by <fcntl.h>, so the change introduces a compile-time failure (“O_RDWR undeclared”/implicit declaration of open). Adding the missing header is required for the daemonization build to succeed.
Useful? React with 👍 / 👎.
c33f728 to
5151ec3
Compare
cosmo0920
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.
We need to modify monkey related codes in https://github.com/monkey/monkey instead of here.
After merging this type of modification there, we can synchronize that kind of changes.
So, could you send your monkey related patch into monkey repo at first?
5151ec3 to
3bc9b47
Compare
|
The lib monkey update is optional. So, I revert the update. Please review the flb_utils_set_daemon function. |
3bc9b47 to
6c1f8d3
Compare
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)
src/flb_utils.c(3 hunks)
🔇 Additional comments (2)
src/flb_utils.c (2)
25-28: LGTM! Headers properly guarded.The conditional inclusion of
fcntl.handunistd.hcorrectly addresses the need foropen(),O_RDWR,dup2(), andclose()declarations used in the daemonization function.
184-184: LGTM!Variable declaration is appropriately scoped for the file descriptor handling below.
6c1f8d3 to
3cc5158
Compare
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: 0
🧹 Nitpick comments (1)
src/flb_utils.c (1)
210-226: Correct implementation of stdio redirection.The logic properly addresses the fd reuse issue by redirecting standard streams to
/dev/null:
- Handles
open()failure appropriately- Checks all
dup2()calls and cleans up on failure- Correctly avoids closing fd when it's already 0, 1, or 2
Optional: Add braces for consistency.
Consider adding braces to the if statement for consistency with project style:
-if (fd > 2) - close(fd); +if (fd > 2) { + close(fd); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_utils.c(3 hunks)
🔇 Additional comments (1)
src/flb_utils.c (1)
25-28: LGTM! Includes properly guarded.The conditional includes are correct and necessary for the daemonization logic.
fcntl.hprovidesO_RDWRandopen(), whileunistd.hprovidesdup2()and the standard file descriptor constants.
3cc5158 to
dcc13b9
Compare
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: 0
🧹 Nitpick comments (1)
src/flb_utils.c (1)
184-227: Daemon stdio redirection via/dev/nulllooks correct; clarify failure semanticsThe
open/dup2sequence andfd > 2close pattern are correct and address the fd‑reuse issue this PR targets. A couple of points to consider:
- This function historically either succeeded or called
exit(). The new paths nowreturn -1onopen/dup2failure after daemonization steps have already run. Callers must be checking the return value; otherwise failures here won’t stop startup.- For debugging/ops, you might want to include the OS error in the log (e.g., using
flb_errno()orstrerror(errno)) so it’s clearer why/dev/nullhandling failed.If call sites already treat non‑zero returns as fatal, the change is fine; otherwise, consider either propagating the error in a way that aborts startup (to match other fatal branches in this function) or documenting the partial‑daemonization behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/flb_utils.c(3 hunks)
🔇 Additional comments (1)
src/flb_utils.c (1)
25-28: Guarded inclusion of<fcntl.h>/<unistd.h>is appropriateThese headers are now correctly pulled in only when
FLB_HAVE_FORKis enabled, matching the use ofopen,O_RDWR,dup2, andSTDIN_FILENO/STDOUT_FILENO/STDERR_FILENOin the daemon code path while avoiding issues on non‑POSIX builds.
…monize Signed-off-by: Yuichiro NAITO <[email protected]>
dcc13b9 to
89f9d98
Compare
While I'm studying the
flb_util_set_daemoncode, I see that stdout and stderr are simply closed and don’t reopened to anything. It is potentially dangerous because the number 1 and 2 file descriptors are reused in the preceding open(2), pipe(2), or socket(2) system calls. The hardcoded write toSTDERR_FILENOalways uses the number 2 file descriptor; however, it will no longer be the terminal. An unexpected write to a pipe, file, or socket will happen. If the peer of the pipe or socket will never read the message, the write(2) system call will eventually be blocked because of the buffer full. This will cause the daemon to hang up.So, the stdin, stdout, and stderr file descriptors should be reopened to the '/dev/null'.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.