Re: [SSSD] [PATHC] 1625-Confusing error messages for invalid sssd.conf

[Jakub Hrozek]

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" 
> 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