[SSSD] [PATCH] DB: Update sss_domain_info with new updated data

Jakub Hrozek jhrozek at redhat.com
Tue Aug 27 09:18:23 UTC 2013


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.

-------------- next part --------------
>From d647b45ebbd78a1c35fe81d2e1267f94a5d99ae4 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
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

-------------- next part --------------
>From 44ff9c5cf6a81497083deaaa8fba1077976b63b9 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
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



More information about the sssd-devel mailing list