On 11/16/2012 04:40 PM, Jan Cholasta wrote:
On 16.11.2012 15:25, Ondrej Kos wrote:
On 11/15/2012 03:03 PM, Jan Cholasta wrote:
On 14.11.2012 16:20, Ondrej Kos wrote:
On 11/14/2012 03:38 PM, Simo Sorce wrote:
On Wed, 2012-11-14 at 15:18 +0100, Jan Cholasta wrote:
Just one more nitpick: SSS_DB_CHECK_PTS and sss_db_version_check are
used only in sysdb.c, so there is no reason to have them defined
publicly in util.h+util.c. Move them both to sysdb.c please (you
might
also want to rename them to better fit in sysdb.c).
Yes please, rename them to be sysdb_* functions and move them in sysdb
where they belong.
The util/ shouldn't have component specific functions.
Simo.
moved all code to sysdb, new patch attached.
O.
OK, perhaps I should have been more clear with that last nitpick:
sysdb_version_check and SYSDB_CHECK_PTS are used only by sysdb.c
internally, there is no need to put them in *any* header file. Please
keep them *only* is sysdb.c and make sysdb_version_check static.
Speaking of SYSDB_CHECK_PTS, I see no point in having it #defined, as it
is used solely by sysdb_version_check and changing its value doesn't
really do anything other than breaking sysdb_version_check.
Also I have done more intense testing and found a few more issues:
1) sss_cache does not print the error message:
# sss_cache -u baduser --debug 10
...
(Thu Nov 15 06:38:56:755800 2012) [sss_cache]
[sysdb_domain_init_internal] (0x0010): Wrong DB version (got 0.13
expected 0.12)
(Thu Nov 15 06:38:56:756043 2012) [sss_cache] [init_domains] (0x0020):
Could not initialize connection to the sysdb
Could not open available domains
(Thu Nov 15 06:38:56:756426 2012) [sss_cache] [init_context] (0x0040):
Initialization of sysdb connections failed
...
This is because sysdb_init is called in two places in sss_cache, but
only one includes SYSDB_MIS_CHECK.
2) The "DB version mismatch!" error message seems redundant, as the
"Unknown DB version ..." or "Wrong DB version ..." debug message is
always printed right before it in both daemons and tools in all (?)
debug levels.
3) I have noticed that the common practice for multi-statement macros in
SSSD is to wrap them in "do { stuff } while(0)". SYSDB_MIS_CHECK should
probably do the same.
4) The "Higher version of database is expected" error message is
actually never printed in SSSD. While this is fine (in this case it is
not really an error, because SSSD can handle this situation well by
upgrading the database), the in_tool check for this in
sysdb_version_mismatch should be fixed.
5) There are some errors in the "remove cache files" message:
a) I think it should say "Removing cache files" instead of "Removing
cache*d* files".
b) The path "/var/run/sss/db" is wrong, use the DB_PATH macro here
instead.
c) The comma in "... be aware, that ..." does not feel right to me.
6) Can we turn the whole sysdb_version_mismatch function into a macro?
Or perhaps two macros, one for use in daemons and the other for use in
tools? I have something like this in mind (also fixes points 2 to 5):
#define SYSDB_VERSION_ERROR_HINT \
ERROR("Removing cache files in "DB_PATH" should fix the issue, " \
"but note that removing cache files will also remove all of
your " \
"cached credentials.\n")
#define SYSDB_VERSION_LOWER_ERROR(ret) do { \
if (ret == EUCLEAN) { \
ERROR("Lower version of database is expected!\n"); \
SYSDB_VERSION_ERROR_HINT; \
} \
} while(0)
#define SYSDB_VERSION_HIGHER_ERROR(ret) do { \
if (ret == EMEDIUMTYPE) { \
ERROR("Higher version of database is expected!\n"); \
SYSDB_VERSION_ERROR_HINT; \
} \
} while(0)
/* use this in daemons */
#define SYSDB_VERSION_ERROR_DAEMON(ret) \
SYSDB_VERSION_LOWER_ERROR(ret)
/* use this in tools */
#define SYSDB_VERSION_ERROR(ret) \
SYSDB_VERSION_LOWER_ERROR(ret); \
SYSDB_VERSION_HIGHER_ERROR(ret)
Honza
great idea.
new patch attached
O.
Thank you.
Replace this:
+ ERROR("Higher version of database is expected!\n"); \
+ SYSDB_VERSION_ERROR_HINT; \
+ ERROR("In this case, you could also try running SSSD, " \
+ "which should handle the database upgrade.\n"); \
with this:
+ ERROR("Higher version of database is expected!\n"); \
+ ERROR("In order to upgrade the database, you must run SSSD.\n"); \
+ SYSDB_VERSION_ERROR_HINT; \
and it's an ACK.
(Also it wouldn't hurt if a native English speaker checked the error
messages for correct grammar.)
Honza
patch attached.
Ondra
--
Ondrej Kos
Associate Software Engineer
Identity Management
Red Hat Czech
phone: +420-532-294-558
cell: +420-736-417-909
ext: 82-62558
loc: 1013 Brno 1 office
irc: okos @ #brno
From e5b3f9c904664a8de5923d8319c28ea922721afe Mon Sep 17 00:00:00 2001
From: Ondrej Kos <o...@redhat.com>
Date: Thu, 8 Nov 2012 14:34:36 +0100
Subject: [PATCH] Display more information on DB version crash
https://fedorahosted.org/sssd/ticket/1589
Added check for determining, whether database version is higher or
lower than expected. To distinguish it from other errors it uses
following retun values (further used for appropriate error message):
EMEDIUMTYPE for lower version than expected
EUCLEAN for higher version than expected
When SSSD or one of it's tools fails on DB version mismatch, new error
message is showed suggesting how to proceed.
---
src/db/sysdb.c | 36 +++++++++++++++++++++++++++++++--
src/db/sysdb.h | 30 +++++++++++++++++++++++++++
src/monitor/monitor.c | 1 +
src/responder/common/responder_common.c | 1 +
src/tools/sss_cache.c | 2 ++
src/tools/sss_seed.c | 1 +
src/tools/tools_util.c | 1 +
7 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/src/db/sysdb.c b/src/db/sysdb.c
index 9685163b3f00cc2a45617072efa353589de111ce..dda288f767022fe97b64e36226c5a505eab5abbe 100644
--- a/src/db/sysdb.c
+++ b/src/db/sysdb.c
@@ -947,6 +947,38 @@ errno_t sysdb_add_to_domain(struct sss_domain_info *domain,
return EOK;
}
+/* Compare versions of sysdb, returns ERRNO accordingly */
+static errno_t
+sysdb_version_check(const char *expected,
+ const char *received)
+{
+ int ret;
+ unsigned int exp_major, exp_minor, recv_major, recv_minor;
+
+ ret = sscanf(expected, "%u.%u", &exp_major, &exp_minor);
+ if (ret != 2) {
+ return EINVAL;
+ }
+ ret = sscanf(received, "%u.%u", &recv_major, &recv_minor);
+ if (ret != 2) {
+ return EINVAL;
+ }
+
+ if (recv_major > exp_major) {
+ return EUCLEAN;
+ } else if (recv_major < exp_major) {
+ return EMEDIUMTYPE;
+ }
+
+ if (recv_minor > exp_minor) {
+ return EUCLEAN;
+ } else if (recv_minor < exp_minor) {
+ return EMEDIUMTYPE;
+ }
+
+ return EOK;
+}
+
int sysdb_domain_init_internal(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *db_path,
@@ -1037,7 +1069,7 @@ int sysdb_domain_init_internal(TALLOC_CTX *mem_ctx,
if (!allow_upgrade) {
DEBUG(0, ("Wrong DB version (got %s expected %s)\n",
version, SYSDB_VERSION));
- ret = EINVAL;
+ ret = sysdb_version_check(SYSDB_VERSION, version);
goto done;
}
@@ -1136,7 +1168,7 @@ int sysdb_domain_init_internal(TALLOC_CTX *mem_ctx,
DEBUG(0,("Unknown DB version [%s], expected [%s] for domain %s!\n",
version?version:"not found", SYSDB_VERSION, domain->name));
- ret = EINVAL;
+ ret = sysdb_version_check(SYSDB_VERSION, version);
goto done;
}
diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 39e0f9f5245c67f3798055823e98c2db0fbdebba..c60f7e9517428f704b714d837579f132a187b69e 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -214,6 +214,36 @@
#define SYSDB_MOD_DEL LDB_FLAG_MOD_DELETE
#define SYSDB_MOD_REP LDB_FLAG_MOD_REPLACE
+/* sysdb version check macros */
+#define SYSDB_VERSION_ERROR_HINT \
+ ERROR("Removing cache files in "DB_PATH" should fix the issue, " \
+ "but note that removing cache files will also remove all of your " \
+ "cached credentials.\n")
+
+#define SYSDB_VERSION_LOWER_ERROR(ret) do { \
+ if (ret == EUCLEAN) { \
+ ERROR("Lower version of database is expected!\n"); \
+ SYSDB_VERSION_ERROR_HINT; \
+ } \
+} while(0)
+
+#define SYSDB_VERSION_HIGHER_ERROR(ret) do { \
+ if (ret == EMEDIUMTYPE) { \
+ ERROR("Higher version of database is expected!\n"); \
+ ERROR("In order to upgrade the database, you must run SSSD.\n"); \
+ SYSDB_VERSION_ERROR_HINT; \
+ } \
+} while(0)
+
+/* use this in daemons */
+#define SYSDB_VERSION_ERROR_DAEMON(ret) \
+ SYSDB_VERSION_LOWER_ERROR(ret)
+
+/* use this in tools */
+#define SYSDB_VERSION_ERROR(ret) \
+ SYSDB_VERSION_LOWER_ERROR(ret); \
+ SYSDB_VERSION_HIGHER_ERROR(ret)
+
struct confdb_ctx;
struct sysdb_ctx;
diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index caa157138c19d18b61a77b1f9d4b29ac9e1fb64e..987a78547c745e8283deeb646342734554f1737a 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -2185,6 +2185,7 @@ int monitor_process_init(struct mt_ctx *ctx,
}
ret = sysdb_init(tmp_ctx, ctx->cdb, NULL, true, &db_list);
if (ret != EOK) {
+ SYSDB_VERSION_ERROR_DAEMON(ret);
return ret;
}
talloc_zfree(tmp_ctx);
diff --git a/src/responder/common/responder_common.c b/src/responder/common/responder_common.c
index d9f73fe2698cd7eacfb1a589c159e5cb037dab64..50705a3f227d38f7789d4adc5a69d246f4b1be29 100644
--- a/src/responder/common/responder_common.c
+++ b/src/responder/common/responder_common.c
@@ -839,6 +839,7 @@ int sss_process_init(TALLOC_CTX *mem_ctx,
ret = sysdb_init(rctx, cdb, NULL, false, &rctx->db_list);
if (ret != EOK) {
+ SYSDB_VERSION_ERROR_DAEMON(ret);
DEBUG(0, ("fatal error initializing resp_ctx\n"));
return ret;
}
diff --git a/src/tools/sss_cache.c b/src/tools/sss_cache.c
index 06b0099dc642812752949aa6cb8781ac1349ad90..368b1df9d42f5152df54b5ae94633e74b3154255 100644
--- a/src/tools/sss_cache.c
+++ b/src/tools/sss_cache.c
@@ -339,6 +339,7 @@ errno_t init_domains(struct cache_tool_ctx *ctx, const char *domain)
ret = sysdb_init_domain_and_sysdb(ctx, ctx->confdb, domain, DB_PATH,
&ctx->domains, &db_ctx);
if (ret != EOK) {
+ SYSDB_VERSION_ERROR(ret);
DEBUG(1, ("Could not initialize connection to the sysdb\n"));
goto fail;
}
@@ -350,6 +351,7 @@ errno_t init_domains(struct cache_tool_ctx *ctx, const char *domain)
}
} else {
ret = sysdb_init(ctx, ctx->confdb, NULL, false, &ctx->sysdb_list);
+ SYSDB_VERSION_ERROR(ret);
if (ret != EOK) {
DEBUG(1, ("Could not initialize connection to the sysdb\n"));
goto fail;
diff --git a/src/tools/sss_seed.c b/src/tools/sss_seed.c
index cd1b263863e44411e7aac8772c3922178ad9dca4..af296c73a868414be1fbfad72673c051f921b625 100644
--- a/src/tools/sss_seed.c
+++ b/src/tools/sss_seed.c
@@ -631,6 +631,7 @@ static int seed_init_db(TALLOC_CTX *mem_ctx,
ret = sysdb_init_domain_and_sysdb(tmp_ctx, confdb, domain_name,
DB_PATH, &domain, &sysdb);
if (ret != EOK) {
+ SYSDB_VERSION_ERROR(ret);
DEBUG(SSSDBG_CRIT_FAILURE,
("Could not initialize connection to domain '%s' in sysdb.%s\n",
domain_name, ret == ENOENT ? " Domain not found." : ""));
diff --git a/src/tools/tools_util.c b/src/tools/tools_util.c
index 99b79f171d82cde5f1c58988949422981e4d5ee3..73e94136c6d274fde60e0fda8d1fdd8c7ef2747e 100644
--- a/src/tools/tools_util.c
+++ b/src/tools/tools_util.c
@@ -57,6 +57,7 @@ static int setup_db(struct tools_ctx *ctx)
ret = sysdb_init_domain_and_sysdb(ctx, ctx->confdb, "local", DB_PATH,
&ctx->local, &ctx->sysdb);
if (ret != EOK) {
+ SYSDB_VERSION_ERROR(ret);
DEBUG(1, ("Could not initialize connection to the sysdb\n"));
return ret;
}
--
1.7.11.7
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
