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(a)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