[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