Conversation
Signed-off-by: Samuel Cabrero <scabrero@suse.com>
Use same logic as the main daemon. Signed-off-by: Samuel Cabrero <scabrero@suse.com>
Vendor provided configuration is installed in /usr/etc/sssd/sssd.conf. Users can override it creating /etc/sssd/sssd.conf, or override defaults dropping config snippets to /etc/sssd/conf.d/ Doc: https://en.opensuse.org/openSUSE:Packaging_UsrEtc Signed-off-by: Samuel Cabrero <scabrero@suse.com>
![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 adds support for openSUSE's UsrEtc layout, allowing SSSD to fall back to a vendor-provided configuration file. The changes are generally well-implemented across the build system and source code. However, I've identified a memory leak in src/monitor/monitor.c due to incorrect handling of talloc allocations when determining the configuration file path. Additionally, there's a minor bug in configure.ac that causes a user-facing notice to display an empty value. I have provided suggestions to address both of these issues.
| [], [enable_vendordir=no]) | ||
| if test "$enable_vendordir" != no; then | ||
| AC_DEFINE(USE_VENDORDIR, 1, [Define if distribution provided configuration files should be used.]) | ||
| AC_MSG_NOTICE([Used vendor dir: $VENDORDIR]) |
There was a problem hiding this comment.
The shell variable for the vendor directory is vendordir, not VENDORDIR. VENDORDIR is the C preprocessor macro. This will result in an empty value being printed in the notice.
AC_MSG_NOTICE([Used vendor dir: $vendordir])
| #ifdef USE_VENDORDIR | ||
| struct stat stats = { 0 } ; | ||
| #endif /* USE_VENDORDIR */ | ||
| config_file = talloc_strdup(tmp_ctx, SSSD_CONFIG_FILE); | ||
| #ifdef USE_VENDORDIR | ||
| if (stat(config_file, &stats) < 0 && errno == ENOENT) { | ||
| config_file = talloc_strdup(tmp_ctx, SSSD_VENDOR_CONFIG_FILE); | ||
| } | ||
| #endif /* USE_VENDORDIR */ |
There was a problem hiding this comment.
This block of code has a memory leak. If stat(config_file, &stats) fails with ENOENT, config_file is re-assigned with a new allocation from talloc_strdup, leaking the memory from the first allocation. It's better to determine the correct config path first, and then allocate memory for it only once to avoid the leak and simplify the code.
const char *path = SSSD_CONFIG_FILE;
#ifdef USE_VENDORDIR
struct stat stats;
if (stat(path, &stats) < 0 && errno == ENOENT) {
path = SSSD_VENDOR_CONFIG_FILE;
}
#endif /* USE_VENDORDIR */
config_file = talloc_strdup(tmp_ctx, path);
ikerexxe
left a comment
ikerexxe
left a comment
There was a problem hiding this comment.
I only added comments to the first instance of each problem.
I'd highly recommend you to create a centralized place to manage this logic. A new file located in src/util/util_config.c would probably be the best location to place this logic. This way we reduce the maintenance burden and the possibility of applying fixes in one place but forgetting about the other
| #endif /* USE_VENDORDIR */ | ||
| config_file = talloc_strdup(tmp_ctx, SSSD_CONFIG_FILE); | ||
| #ifdef USE_VENDORDIR | ||
| if (stat(config_file, &stats) < 0 && errno == ENOENT) { |
There was a problem hiding this comment.
You should check the return value for config_file before using it for anything
| config_file = talloc_strdup(tmp_ctx, SSSD_CONFIG_FILE); | ||
| #ifdef USE_VENDORDIR | ||
| if (stat(config_file, &stats) < 0 && errno == ENOENT) { | ||
| config_file = talloc_strdup(tmp_ctx, SSSD_VENDOR_CONFIG_FILE); |
There was a problem hiding this comment.
Since you are overwriting the content of config_file it would be sensible to free it before. I know talloc already takes care of this, but it would improve the readability
| #ifdef USE_VENDORDIR | ||
| if (stat(config_file, &stats) < 0 && errno == ENOENT) { | ||
| config_file = talloc_strdup(tmp_ctx, SSSD_VENDOR_CONFIG_FILE); | ||
| } |
There was a problem hiding this comment.
You should add some debugging to state when the user vendor config is used
| AC_SUBST(vendordir) | ||
|
|
||
| AC_ARG_ENABLE([vendordir], | ||
| [AS_HELP_STRING([--enable-vendordir], [Enable support for distribution provided configuration files])], |
There was a problem hiding this comment.
Why are you providing two different options? This is quite confusing
4 participants
To support transactional-updates in openSUSE, this PR adds support for UsrEtc.
Doc: https://en.opensuse.org/openSUSE:Packaging_UsrEtc