sssd: add ldb-modules-path#8116
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to override the LDB modules path at build time via a configure option. The implementation is mostly correct, but the core logic in sss_ldb_init_modules_path has a potential thread-safety issue and uses fprintf to stderr for error reporting, which is not suitable for daemonized processes. I've provided a suggestion to address these points by using an atomic flag for initialization and the SSSD logging infrastructure for error messages.
286911b to
1535eb1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new configure option, --with-ldb-modules-path, to allow overriding the LDB_MODULES_PATH environment variable at build time. The implementation is well-structured, adding the necessary autotools macros and a new C source file to handle setting the environment variable. The logic to set the variable is correctly placed in confdb_init and is made thread-safe using an atomic_flag. My review found one high-severity issue: the implementation does not override an existing LDB_MODULES_PATH environment variable, which contradicts the documented behavior of the new option. A code suggestion has been provided to fix this.
|
Hi,
what is your platform / use case? |
Hi, This is a fix that can be applied to many systems currently. Best Regards |
680781a to
074b094
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds a new configure option --with-ldb-modules-path to override the LDB_MODULES_PATH environment variable. This is a useful feature for custom builds. The implementation is generally sound, integrating well with the autotools build system and initializing the environment variable at an appropriate point. However, I've identified a critical thread-safety issue in the new initialization logic that could lead to a race condition and undefined behavior. My review includes a detailed comment and a code suggestion to resolve this issue.
There was a problem hiding this comment.
Code Review
This pull request introduces a new configure option --with-ldb-modules-path to allow overriding the LDB_MODULES_PATH at build time. The changes correctly add the configure logic and call the initialization function at the right place. However, the implementation of the initialization function has a critical flaw that prevents it from working as intended, and it is also not thread-safe. My review includes a suggestion to fix both issues.
|
Please add a Line 15 in 481700d |
Thank you. But this should be included directly into commit message (not merely in the github PR description) - project has an automation to generate release notes automatically from git history. |
Hi, Sorry I misunderstood, I'll correct that. Best Regards |
alexey-tikhonov
added
Waiting for review
coverity
Hi, would something like bye, |
Hi, In nixOS build-time path from pkg-config would differ from the runtime path due to store-based packaging. Best Regards |
Hi, thanks for the explanation. It would be nice if nixOS developers would consider to fix this so that bye,
|
Hi, I'll take the time to look into it, but I don't think the two are incompatible. Best Regards |
alexey-tikhonov
added
Changes requested
and removed
Waiting for review
coverity
|
Hi, @sumit-bose Sorry for the delay in updating. Best Regards |
sumit-bose
left a comment
sumit-bose
left a comment
There was a problem hiding this comment.
Hi,
thank you for the updates, I have no further comments, ACK.
bye,
Sumit
f5d64b3 to
b854636
Compare
|
Note: Covscan is green. |
Add configure option to specify LDB modules path at build time. This allows distributions to set the correct path when LDB modules are installed in non-standard locations. The path is set once during confdb initialization if configured, respecting any pre-existing LDB_MODULES_PATH environment variable. :packaging: Add --with-ldb-modules-path=PATH configure option to specify LDB modules path at build time. Reviewed-by: Alexey Tikhonov <[email protected]> Reviewed-by: Sumit Bose <[email protected]>
4 participants
Hi,
This PR adds a configure option to specify LDB modules path at build time:
./configure --with-ldb-modules-path="/custom/path1:/custom/path2"This fixes the "Module [memberof] not found" issue that occurs when LDB modules are installed in non-standard locations. Instead of requiring each distribution to maintain their own patch, this provides a standard way to handle different installation paths.
The implementation:
:packaging: Add --with-ldb-modules-path=PATH configure option to specify LDB modules path at build time.
Best regards