Conversation
Why: operators use symlinked config snippets and expect them to be treated like regular config paths. Impact: include processing now resolves symlink metadata before classification. Before/After: before, symlinked include entries could be rejected on some platforms; after, they resolve and parse like regular files. Technical Overview: - use lstat to detect symlink entries during IncludeConfig expansion - resolve symlink targets with stat for file vs directory classification - preserve existing error handling for missing optional files - keep include order semantics unchanged Refs: rsyslog#6517 With the help of AI-Agents: ChatGPT
Summary of ChangesHello @rgerhards, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical issue in rsyslog where the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for symbolic links in $IncludeConfig paths, which is a good improvement. The implementation correctly uses lstat and stat to handle symlinks.
My main feedback is about an error handling logic bug. When an optional include fails (e.g., a broken symlink), the current implementation aborts processing all subsequent include files silently. This can lead to unexpected behavior and hard-to-debug configuration issues. I've suggested a fix to continue to the next file in such cases, which is the expected behavior for optional includes.
| if (stat(cfgFile, &fileInfo) != 0) { | ||
| if (optional == 0) { | ||
| rs_strerror_r(errno, errStr, sizeof(errStr)); | ||
| if (getcwd(cwdBuf, sizeof(cwdBuf)) == NULL) strcpy(cwdBuf, "??getcwd() failed??"); | ||
| parser_errmsg( | ||
| "error accessing config file or directory '%s' " | ||
| "[cwd: %s]: %s", | ||
| cfgFile, cwdBuf, errStr); | ||
| ret = 1; | ||
| } | ||
| goto done; | ||
| } |
There was a problem hiding this comment.
There's a logic bug here. If stat() fails for an optional include (e.g., a broken symlink), the goto done; will cause the function to exit prematurely, silently skipping the rest of the include files. For optional includes, it should continue to the next file in the loop instead of aborting the entire include process.
This logic is copied from the lstat error handling a few lines above, which has the same issue. While a larger refactoring could address the code duplication, fixing this bug in the new code is important.
if (stat(cfgFile, &fileInfo) != 0) {
if (optional == 0) {
rs_strerror_r(errno, errStr, sizeof(errStr));
if (getcwd(cwdBuf, sizeof(cwdBuf)) == NULL) strcpy(cwdBuf, "??getcwd() failed??");
parser_errmsg(
"error accessing config file or directory '%s' "
"[cwd: %s]: %s",
cfgFile, cwdBuf, errStr);
ret = 1;
goto done;
}
continue;
}### Motivation - Include processing failed to handle symbolic links when expanding `$IncludeConfig` paths, causing rsyslog to abort on setups that use symlinked config snippets (see rsyslog#6517). ### Description - Updated `grammar/rainerscript.c` (`cnfDoInclude`) to use `lstat()` on matched paths so symlink entries are detected. - When an entry is a symlink, the code now resolves the target with `stat()` and classifies the target as a regular file or directory before inclusion. - Preserved existing error and optional-file handling and the include ordering semantics. - Modified file: `grammar/rainerscript.c`. ### Testing - Ran code formatting via `devtools/format-code.sh`, which completed successfully. - Attempted `make -j$(nproc) check TESTS=""` in this environment and it failed because there is no `check` make target here (no full build/test run available). - Static checks (searches and file inspection) were used to validate the inserted logic locally.
Motivation
$IncludeConfigpaths, causing rsyslog to abort on setups that use symlinked config snippets (see [General]: rsyslog fails to start when rsyslog.conf / rsyslog.d/* are symbolic links rsyslog/rsyslog#6517).Description
grammar/rainerscript.c(cnfDoInclude) to uselstat()on matched paths so symlink entries are detected.stat()and classifies the target as a regular file or directory before inclusion.grammar/rainerscript.c.Testing
devtools/format-code.sh, which completed successfully.make -j$(nproc) check TESTS=""in this environment and it failed because there is nocheckmake target here (no full build/test run available).Codex Task