On Mon, Aug 26, 2013 at 04:34:14PM +0200, Sumit Bose wrote: > On Thu, Aug 22, 2013 at 11:50:27AM +0200, Jakub Hrozek wrote: > > On Mon, Aug 19, 2013 at 08:14:03AM -0400, Simo Sorce wrote: > > > On Mon, 2013-08-19 at 11:33 +0200, Sumit Bose wrote: > > > > On Sun, Aug 18, 2013 at 10:26:46PM +0200, Jakub Hrozek wrote: > > > > > There seems to be a logic bug in sysdb_master_domain_add_info(). We > > > > > first update the domain data with sysdb search results and then update > > > > > sysdb. I think it should be the other way around. > > > > > > > > iirc the idea was to read the sysdb entry first to make sure we have the > > > > latest data from sysdb. Because of the asynchronous nature of the > > > > request it might be possible that another request already updated the > > > > sysdb entry. To avoid unneeded writes we try to compare the new data > > > > with the latest data we know. This is led by the assumption that > > > > reading from sysdb is a lot cheaper then writing to it. > > > > > > How often does this operation happen ? > > > Simo. > > > > Never until now because it didn't work :-) But if there's a cheap way to > > optimize, then why not. I think the code should just make it clear > > what's happening. > > > > Attached is a new patch that explains why we call the domain update also > > before the sysdb update and adds another domain update after the sysdb > > update to make sure the sss_domain_info structure has the latest data. > > > After a bit more thinking I think the first version of the patch is the > better one. The main reason is that a change in the value will only > happen once for an IPA server and never for AD. When running > ipa-adtrust-install the IPA domain will get its SID and flat name and a > running SSSD will detect the change and update his structures. Since it > is a rare event I think it's acceptable to let parallel running task to > update the domain entry twice because in the general case the call to > sysdb_master_domain_update() is not needed because the server data and > the data in the struct will be the same.
OK. I don't really mind either way but attached is the first version again. > > Additionally both callers set the realm to NULL, i.e. it is not updated > at all because it is derived from the configuration for the master > domains. Would you mind to just remove this parameter? Yes, that's in a separate patch so that the changes can be seen logically.
>From d647b45ebbd78a1c35fe81d2e1267f94a5d99ae4 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> Date: Sat, 17 Aug 2013 09:12:03 +0200 Subject: [PATCH 1/2] DB: Update sss_domain_info with new updated data --- src/db/sysdb_subdomains.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index 58df470182ea6ecbb5b48e288ab6a3d3bcec18ba..c89e4569492fcfb0c7984c1c983a8d4d39e5cf75 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -264,11 +264,6 @@ errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, return ENOMEM; } - ret = sysdb_master_domain_update(domain); - if (ret != EOK) { - goto done; - } - msg = ldb_msg_new(tmp_ctx); if (msg == NULL) { ret = ENOMEM; @@ -350,6 +345,11 @@ errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, goto done; } + ret = sysdb_master_domain_update(domain); + if (ret != EOK) { + goto done; + } + ret = EOK; done: -- 1.8.3.1
>From 44ff9c5cf6a81497083deaaa8fba1077976b63b9 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek <[email protected]> Date: Tue, 27 Aug 2013 11:09:58 +0200 Subject: [PATCH 2/2] DB: remove unused realm parameter from sysdb_master_domain_add_info The parameter was not used at all. --- src/db/sysdb.h | 3 +-- src/db/sysdb_subdomains.c | 21 +-------------------- src/providers/ad/ad_subdomains.c | 2 +- src/providers/ipa/ipa_subdomains.c | 2 +- 4 files changed, 4 insertions(+), 24 deletions(-) diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 96679007af90ee813a580094dafd64cde976fa39..953b15d2309e1a57a0938c475f22e45111796046 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -377,8 +377,7 @@ errno_t sysdb_update_subdomains(struct sss_domain_info *domain); errno_t sysdb_master_domain_update(struct sss_domain_info *domain); errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, - const char *realm, const char *flat, - const char *id); + const char *flat, const char *id); errno_t sysdb_subdomain_delete(struct sysdb_ctx *sysdb, const char *name); diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c index c89e4569492fcfb0c7984c1c983a8d4d39e5cf75..0e7514baa993a4d02402695bf51992488fc0ed1e 100644 --- a/src/db/sysdb_subdomains.c +++ b/src/db/sysdb_subdomains.c @@ -251,8 +251,7 @@ done: } errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, - const char *realm, const char *flat, - const char *id) + const char *flat, const char *id) { TALLOC_CTX *tmp_ctx; struct ldb_message *msg; @@ -277,24 +276,6 @@ errno_t sysdb_master_domain_add_info(struct sss_domain_info *domain, goto done; } - if (realm != NULL && (domain->realm == NULL || - strcmp(domain->realm, realm) != 0)) { - ret = ldb_msg_add_empty(msg, SYSDB_SUBDOMAIN_REALM, - LDB_FLAG_MOD_REPLACE, NULL); - if (ret != LDB_SUCCESS) { - ret = sysdb_error_to_errno(ret); - goto done; - } - - ret = ldb_msg_add_string(msg, SYSDB_SUBDOMAIN_REALM, realm); - if (ret != LDB_SUCCESS) { - ret = sysdb_error_to_errno(ret); - goto done; - } - - do_update = true; - } - if (flat != NULL && (domain->flat_name == NULL || strcmp(domain->flat_name, flat) != 0)) { ret = ldb_msg_add_empty(msg, SYSDB_SUBDOMAIN_FLAT, diff --git a/src/providers/ad/ad_subdomains.c b/src/providers/ad/ad_subdomains.c index be4781cc5bc01152ffa1fbedf0ad5352f72939ae..0eebd4d98bb59ab1bdc65fabc2edef821b4d3b73 100644 --- a/src/providers/ad/ad_subdomains.c +++ b/src/providers/ad/ad_subdomains.c @@ -569,7 +569,7 @@ static void ad_subdomains_get_netlogon_done(struct tevent_req *req) DEBUG(SSSDBG_TRACE_FUNC, ("Found flat name [%s].\n", ctx->flat_name)); ret = sysdb_master_domain_add_info(ctx->sd_ctx->be_ctx->domain, - NULL, ctx->flat_name, ctx->master_sid); + ctx->flat_name, ctx->master_sid); ret = ad_subdomains_get_slave(ctx); if (ret == EAGAIN) { diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c index 9ded9954bbc819e65e3b222c8968d2440320c4be..1a0c1c1bbf5f4e6fa44b352af4883f8624074ad5 100644 --- a/src/providers/ipa/ipa_subdomains.c +++ b/src/providers/ipa/ipa_subdomains.c @@ -901,7 +901,7 @@ static void ipa_subdomains_handler_master_done(struct tevent_req *req) } ret = sysdb_master_domain_add_info(ctx->sd_ctx->be_ctx->domain, - NULL, flat, id); + flat, id); } else { ctx->search_base_iter++; ret = ipa_subdomains_handler_get(ctx, IPA_SUBDOMAINS_MASTER); -- 1.8.3.1
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
