[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