On Thu, Aug 25, 2016 at 1:44 PM, Jakub Hrozek <[email protected]> wrote: > On Wed, Aug 17, 2016 at 01:13:16PM +0200, Fabiano Fidêncio wrote: >> This patchset resolves https://fedorahosted.org/sssd/ticket/3128 >> >> CI has passed: http://sssd-ci.duckdns.org/logs/job/51/84/summary.html >> >> Best Regards, >> -- >> Fabiano Fidêncio > >> From fdef2d959af54e2208a3de86c1d3d7b03c58cce1 Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <[email protected]> >> Date: Tue, 16 Aug 2016 11:20:49 +0200 >> Subject: [PATCH 1/2] SYSDB: Rework sysdb_cache_connect() > > ACK > >> From e4e232ea548119c140529857dd43dbebf31c627e Mon Sep 17 00:00:00 2001 >> From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <[email protected]> >> Date: Tue, 16 Aug 2016 11:46:41 +0200 >> Subject: [PATCH 2/2] SYSDB: Remove the timestamp cache for a newly created > > The patch works fine, thank you. I just have one question related to > code-style, see inline. > >> static errno_t sysdb_cache_connect_helper(TALLOC_CTX *mem_ctx, >> + struct sysdb_ctx *sysdb, >> struct sss_domain_info *domain, >> const char *ldb_file, >> int flags, >> const char *exp_version, >> const char *base_ldif, >> + bool >> remove_ts_cache_when_newly_created, >> struct ldb_context **_ldb, >> const char **_version) >> { >> @@ -527,6 +545,7 @@ static errno_t sysdb_cache_connect_helper(TALLOC_CTX >> *mem_ctx, >> const char *version = NULL; >> int ret; >> struct ldb_context *ldb; >> + bool ldb_file_exists; >> >> tmp_ctx = talloc_new(NULL); >> if (!tmp_ctx) { >> @@ -534,6 +553,8 @@ static errno_t sysdb_cache_connect_helper(TALLOC_CTX >> *mem_ctx, >> goto done; >> } >> >> + ldb_file_exists = !(access(ldb_file, F_OK) == -1 && errno == ENOENT); >> + >> ret = sysdb_ldb_connect(tmp_ctx, ldb_file, flags, &ldb); >> if (ret != EOK) { >> DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_ldb_connect failed.\n"); >> @@ -592,8 +613,18 @@ static errno_t sysdb_cache_connect_helper(TALLOC_CTX >> *mem_ctx, >> goto done; >> } >> >> - /* The cache has been newly created. >> - * We need to reopen the LDB to ensure that >> + /* The cache has been newly created. */ >> + if (remove_ts_cache_when_newly_created && !ldb_file_exists) { >> + ret = remove_ts_cache(sysdb); >> + if (ret != EOK) { >> + DEBUG(SSSDBG_MINOR_FAILURE, >> + "Could not delete the timestamp ldb file (%d) (%s)\n", >> + ret, sss_strerror(ret)); >> + goto done; >> + } >> + } >> + > Wouldn't it make more sense to move the logic outside to the caller > (sysdb_cache_connect) ? that way the helper could just really help > connect to database and the cache removal logic would be in the > ldb-cache specific sysdb_cache_connect() function.
Hmm. It's a valid approach and, personally, I don't have a strong preference about where this logic should be. You can find the v2 attached to this email. One test failed on Debian, not sure whether it's related though :-\ http://sssd-ci.duckdns.org/logs/job/52/39/summary.html Best Regards, -- Fabiano Fidêncio
From 13920454a71d687cd0f5b17e698ae98a53f613d1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <[email protected]> Date: Tue, 16 Aug 2016 11:20:49 +0200 Subject: [PATCH v2 1/2] SYSDB: Rework sysdb_cache_connect() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As sysdb_cache_connect() has two very specific use cases (connect to the cache and connect to the timestamp cache) and each of those calls have a predetermined/fixed sets of values for a few parameters, let's try to make the code a bit simpler to follow by having explicit functions for connecting to the cache and connecting to the timestamp cache. Macros could be used as well, but I have a slightly preference for having two new functions instead of macros accessing internal parameters of the macro's parameter. Related: https://fedorahosted.org/sssd/ticket/3128 Signed-off-by: Fabiano Fidêncio <[email protected]> --- src/db/sysdb_init.c | 53 ++++++++++++++++++++++++++++++++++------------------- 1 file changed, 34 insertions(+), 19 deletions(-) diff --git a/src/db/sysdb_init.c b/src/db/sysdb_init.c index 9e3646b..5993470 100644 --- a/src/db/sysdb_init.c +++ b/src/db/sysdb_init.c @@ -511,14 +511,14 @@ done: return ret; } -static errno_t sysdb_cache_connect(TALLOC_CTX *mem_ctx, - struct sss_domain_info *domain, - const char *ldb_file, - int flags, - const char *exp_version, - const char *base_ldif, - struct ldb_context **_ldb, - const char **_version) +static errno_t sysdb_cache_connect_helper(TALLOC_CTX *mem_ctx, + struct sss_domain_info *domain, + const char *ldb_file, + int flags, + const char *exp_version, + const char *base_ldif, + struct ldb_context **_ldb, + const char **_version) { TALLOC_CTX *tmp_ctx = NULL; struct ldb_message_element *el; @@ -619,6 +619,29 @@ done: return ret; } +static errno_t sysdb_cache_connect(TALLOC_CTX *mem_ctx, + struct sysdb_ctx *sysdb, + struct sss_domain_info *domain, + struct ldb_context **ldb, + const char **version) +{ + return sysdb_cache_connect_helper(mem_ctx, domain, sysdb->ldb_file, + 0, SYSDB_VERSION, SYSDB_BASE_LDIF, + ldb, version); +} + +static errno_t sysdb_ts_cache_connect(TALLOC_CTX *mem_ctx, + struct sysdb_ctx *sysdb, + struct sss_domain_info *domain, + struct ldb_context **ldb, + const char **version) +{ + return sysdb_cache_connect_helper(mem_ctx, domain, sysdb->ldb_ts_file, + LDB_FLG_NOSYNC, SYSDB_TS_VERSION, + SYSDB_TS_BASE_LDIF, + ldb, version); +} + static errno_t remove_ts_cache(struct sysdb_ctx *sysdb) { errno_t ret; @@ -649,9 +672,7 @@ static int sysdb_domain_cache_connect(struct sysdb_ctx *sysdb, return ENOMEM; } - ret = sysdb_cache_connect(tmp_ctx, domain, sysdb->ldb_file, 0, - SYSDB_VERSION, SYSDB_BASE_LDIF, - &ldb, &version); + ret = sysdb_cache_connect(tmp_ctx, sysdb, domain, &ldb, &version); switch (ret) { case ERR_SYSDB_VERSION_TOO_OLD: if (upgrade_ctx == NULL) { @@ -731,10 +752,7 @@ static int sysdb_timestamp_cache_connect(struct sysdb_ctx *sysdb, return ENOMEM; } - ret = sysdb_cache_connect(tmp_ctx, domain, - sysdb->ldb_ts_file, LDB_FLG_NOSYNC, - SYSDB_TS_VERSION, SYSDB_TS_BASE_LDIF, - &ldb, &version); + ret = sysdb_ts_cache_connect(tmp_ctx, sysdb, domain, &ldb, &version); switch (ret) { case ERR_SYSDB_VERSION_TOO_OLD: if (upgrade_ctx == NULL) { @@ -801,10 +819,7 @@ static int sysdb_timestamp_cache_connect(struct sysdb_ctx *sysdb, /* Now the connect must succeed because the previous cache doesn't * exist anymore. */ - ret = sysdb_cache_connect(tmp_ctx, domain, - sysdb->ldb_ts_file, LDB_FLG_NOSYNC, - SYSDB_TS_VERSION, SYSDB_TS_BASE_LDIF, - &ldb, &version); + ret = sysdb_ts_cache_connect(tmp_ctx, sysdb, domain, &ldb, &version); if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Could not delete the timestamp ldb file (%d) (%s)\n", -- 2.7.4
From 318d97d6f9c9920a61b57d63a0a08a75733b1ee8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabiano=20Fid=C3=AAncio?= <[email protected]> Date: Tue, 16 Aug 2016 11:46:41 +0200 Subject: [PATCH v2 2/2] SYSDB: Remove the timestamp cache for a newly created cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As many users are used to remove the persistent cache without removing the timestamp cache, let's throw away the timestamp cache in this case. Resolves: https://fedorahosted.org/sssd/ticket/3128 Signed-off-by: Fabiano Fidêncio <[email protected]> --- src/db/sysdb_init.c | 69 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 22 deletions(-) diff --git a/src/db/sysdb_init.c b/src/db/sysdb_init.c index 5993470..c387c1b 100644 --- a/src/db/sysdb_init.c +++ b/src/db/sysdb_init.c @@ -511,12 +511,30 @@ done: return ret; } +static errno_t remove_ts_cache(struct sysdb_ctx *sysdb) +{ + errno_t ret; + + if (sysdb->ldb_ts_file == NULL) { + return EOK; + } + + ret = unlink(sysdb->ldb_ts_file); + if (ret != EOK && errno != ENOENT) { + return errno; + } + + return EOK; +} + static errno_t sysdb_cache_connect_helper(TALLOC_CTX *mem_ctx, + struct sysdb_ctx *sysdb, struct sss_domain_info *domain, const char *ldb_file, int flags, const char *exp_version, const char *base_ldif, + bool *_newly_created, struct ldb_context **_ldb, const char **_version) { @@ -527,6 +545,7 @@ static errno_t sysdb_cache_connect_helper(TALLOC_CTX *mem_ctx, const char *version = NULL; int ret; struct ldb_context *ldb; + bool newly_created; tmp_ctx = talloc_new(NULL); if (!tmp_ctx) { @@ -592,8 +611,9 @@ static errno_t sysdb_cache_connect_helper(TALLOC_CTX *mem_ctx, goto done; } - /* The cache has been newly created. - * We need to reopen the LDB to ensure that + newly_created = true; + + /* We need to reopen the LDB to ensure that * all of the special values take effect * (such as enabling the memberOf plugin and * the various indexes). @@ -613,6 +633,9 @@ static errno_t sysdb_cache_connect_helper(TALLOC_CTX *mem_ctx, } done: if (ret == EOK) { + if (_newly_created != NULL) { + *_newly_created = newly_created; + } *_ldb = talloc_steal(mem_ctx, ldb); } talloc_free(tmp_ctx); @@ -625,9 +648,27 @@ static errno_t sysdb_cache_connect(TALLOC_CTX *mem_ctx, struct ldb_context **ldb, const char **version) { - return sysdb_cache_connect_helper(mem_ctx, domain, sysdb->ldb_file, + bool newly_created; + bool ldb_file_exists; + errno_t ret; + + ldb_file_exists = !(access(sysdb->ldb_file, F_OK) == -1 && errno == ENOENT); + + ret = sysdb_cache_connect_helper(mem_ctx, sysdb, domain, sysdb->ldb_file, 0, SYSDB_VERSION, SYSDB_BASE_LDIF, - ldb, version); + &newly_created, ldb, version); + + /* The cache has been newly created. */ + if (ret == EOK && newly_created && !ldb_file_exists) { + ret = remove_ts_cache(sysdb); + if (ret != EOK) { + DEBUG(SSSDBG_MINOR_FAILURE, + "Could not delete the timestamp ldb file (%d) (%s)\n", + ret, sss_strerror(ret)); + } + } + + return ret; } static errno_t sysdb_ts_cache_connect(TALLOC_CTX *mem_ctx, @@ -636,28 +677,12 @@ static errno_t sysdb_ts_cache_connect(TALLOC_CTX *mem_ctx, struct ldb_context **ldb, const char **version) { - return sysdb_cache_connect_helper(mem_ctx, domain, sysdb->ldb_ts_file, + return sysdb_cache_connect_helper(mem_ctx, sysdb, domain, sysdb->ldb_ts_file, LDB_FLG_NOSYNC, SYSDB_TS_VERSION, - SYSDB_TS_BASE_LDIF, + SYSDB_TS_BASE_LDIF, NULL, ldb, version); } -static errno_t remove_ts_cache(struct sysdb_ctx *sysdb) -{ - errno_t ret; - - if (sysdb->ldb_ts_file == NULL) { - return EOK; - } - - ret = unlink(sysdb->ldb_ts_file); - if (ret != EOK && errno != ENOENT) { - return errno; - } - - return EOK; -} - static int sysdb_domain_cache_connect(struct sysdb_ctx *sysdb, struct sss_domain_info *domain, struct sysdb_dom_upgrade_ctx *upgrade_ctx) -- 2.7.4
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
