[SSSD] [PATHC] 1625-Confusing error messages for invalid sssd.conf
Jakub Hrozek
jhrozek at redhat.com
Wed Mar 20 13:16:23 UTC 2013
On Thu, Nov 29, 2012 at 09:33:08AM -0500, Ariel Barria wrote:
Hi Ariel,
this is a good start!
> From 182c5c70d52734caba59d97ca318b036412a6705 Mon Sep 17 00:00:00 2001
> From: "Ariel O. Barria" <arielb at fedoraproject.org>
> Date: Tue, 27 Nov 2012 19:21:05 -0500
> Subject: [PATCH] Confusing error messages for invalid sssd.conf
>
> https://fedorahosted.org/sssd/ticket/1625
> Amending errors messages and add other error codes to be more specific
> and avoid confusion.
> ---
> src/monitor/monitor.c | 32 ++++++++++++++++++++++++--------
> 1 file changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
> index 71f915723426e97aaf98c2bccee81a41e7595da8..d9bee5b3b13854841dbaabea19c3db2257df9a48 100644
> --- a/src/monitor/monitor.c
> +++ b/src/monitor/monitor.c
> @@ -2752,15 +2752,31 @@ int main(int argc, const char *argv[])
> /* Parse config file, fail if cannot be done */
> ret = load_configuration(tmp_ctx, config_file, &monitor);
> if (ret != EOK) {
> - if (ret == EPERM) {
> - DEBUG(1, ("Cannot read configuration file %s\n", config_file));
> + switch (ret) {
> + case EPERM:
> + DEBUG(SSSDBG_CRIT_FAILURE,
> + ("Configuration file %s mode bits too permissive.\n",
> + config_file));
I think we should rather say "Not enough permission to read
configuration file" because maybe EPERM could be returned in case
SELinux denied access as well.
> sss_log(SSS_LOG_ALERT,
> - "Cannot read config file %s, please check if permissions "
> - "are 0600 and the file is owned by root.root", config_file);
> - } else {
> - DEBUG(SSSDBG_CRIT_FAILURE, ("Error loading configuration database: "
> - "[%d]: %s\n", ret, strerror(ret)));
> - sss_log(SSS_LOG_ALERT, "Cannot load configuration database");
> + "Cannot read config file %s, please check if permissions "
> + "are 0600 and the file is owned by root.root", config_file);
> + break;
I think we should only special-case EPERM and not the other error codes
because they are quite generic. I think that for anything else other
than EPERM we should simply say that SSSD couldn't load the
configuration database.
Longer-term, this might be a perfect place to introduce new error codes.
> + case ENOENT:
> + DEBUG(SSSDBG_CRIT_FAILURE, ("Error loading configuration file %s: "
> + "invalid configuration.\n", config_file));
> + sss_log(SSS_LOG_ALERT, "Cannot load configuration file.");
> + break;
> + case EINVAL:
> + DEBUG(SSSDBG_CRIT_FAILURE, ("Error loading configuration file %s: "
> + "empty file.\n", config_file));
> + sss_log(SSS_LOG_ALERT, "Cannot load configuration file.");
> + break;
> + default:
> + DEBUG(SSSDBG_CRIT_FAILURE, ("Error loading configuration file %s.",
> + config_file));
> + sss_log(SSS_LOG_ALERT, "Cannot load configuration file.[%d]: %s\n",
> + ret, strerror(ret));
> + break;
> }
> return 4;
> }
> --
> 1.7.11.7
>
More information about the sssd-devel
mailing list