On Fri, Aug 26, 2016 at 08:09:56AM +0200, Fabiano Fidêncio wrote: > 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.
Thanks, I like this code better. I tested by removing the persistent cache, leaving the timestamp cache and requesting a user. The timestamp cache was recreated and the stamps matched those in the synchronous cache. ACK > > One test failed on Debian, not sure whether it's related though :-\ > http://sssd-ci.duckdns.org/logs/job/52/39/summary.html I don't think it's realted, nonetheless, I just sent the patches to CI again and will push them once the results are back. _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
