On Tue, 2012-07-31 at 13:25 +0200, Jakub Hrozek wrote:
> On Mon, Jul 23, 2012 at 05:38:50PM -0400, Simo Sorce wrote:
> > As aprt of the work for ticket #1380 I have created a long patchset that
> > cleans up and refactors some of the ipa subdomain work.
> >
> > This is in order to be able to refresh subdomains on a periodic basis
> > and not just when the nss service pings in and also to provide a place
> > where to trigger an update of the krb5.conf file only when necessary.
> >
> > The patchset is long (16 patches) but each patch should be small enough
> > to be easy to review.
> >
> > The patches have been partially tested, the internal sysdb tests pass,
> > and I did test on a setup environment, but the setup env was not up to
> > date, so the test was not complete.
> >
> > However I am confident that they should work (last famous word) and I am
> > posting them so that Jakub can take over the second part of the work and
> > add the code that actually saves the domain_realm info in a file.
> >
> > Simo.
> >
>
> I tested the patches and they work good.
>
> In the "Change refreshing of subdomains" patch, what is the purpose of
> "handled"? It's never assigned to and it's declared using the C-99 dynamic
> array feature.
>
> The patches only change to using ldb_msg_add_string with bare strings in
> the subdomains code. We should also check other modules, like sysdb_ops.c
>
> The rest looks good to me and works fine, so once the "handled" variable
> is removed or fixed, I'll ack.
Thanks a lot for pointing this out.
Turns out for some reason the line that actually sets handled elements
went missing.
I revved up just that patch, here it is.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
>From 862970b77b36b20f1df02d55682fa7ed3bff03c6 Mon Sep 17 00:00:00 2001
From: Simo Sorce <s...@redhat.com>
Date: Fri, 20 Jul 2012 12:36:43 -0400
Subject: [PATCH] Change refreshing of subdomains
This patch keeps a local copy of the subdomains in the ipa subdomains plugin
context.
This has 2 advantages:
1. allows to check if anything changed w/o always hitting the sysdb.
2. later will allows us to dump this information w/o having to retrieve it
again. The timestamp also allows to avoid refreshing too often.
---
src/db/sysdb.h | 3 +-
src/db/sysdb_subdomains.c | 13 ++-
src/providers/data_provider_be.c | 1 -
src/providers/dp_backend.h | 1 -
src/providers/ipa/ipa_subdomains.c | 225 +++++++++++++++++++++++++-----------
src/tests/sysdb-tests.c | 36 +++---
6 files changed, 187 insertions(+), 92 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 0e2404c70c58970d7533c5604c65babb40630c70..43ac61c2132c18db9beccf68e488e0132a98d58c 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -371,7 +371,8 @@ errno_t sysdb_get_subdomains(TALLOC_CTX *mem_ctx,
errno_t sysdb_domain_create(struct sysdb_ctx *sysdb, const char *domain_name);
errno_t sysdb_update_subdomains(struct sysdb_ctx *sysdb,
- struct sysdb_subdom **subdomains);
+ int num_subdoms,
+ struct sysdb_subdom *subdoms);
errno_t sysdb_get_subdomain_context(TALLOC_CTX *mem_ctx,
struct sysdb_ctx *sysdb,
diff --git a/src/db/sysdb_subdomains.c b/src/db/sysdb_subdomains.c
index 8f1df88be0d6c4f97f7bb7a6ba367822980b58d3..8489041bd7763c3b094939f56ca10df3b4f90b6e 100644
--- a/src/db/sysdb_subdomains.c
+++ b/src/db/sysdb_subdomains.c
@@ -430,7 +430,8 @@ done:
}
errno_t sysdb_update_subdomains(struct sysdb_ctx *sysdb,
- struct sysdb_subdom **subdomains)
+ int num_subdoms,
+ struct sysdb_subdom *subdoms)
{
int ret;
int sret;
@@ -475,9 +476,9 @@ errno_t sysdb_update_subdomains(struct sysdb_ctx *sysdb,
* - if a subdomain already exists in sysdb, mark it for preservation
* - if the subdomain doesn't exist in sysdb, create its bare structure
*/
- for (c = 0; subdomains[c] != NULL; c++) {
+ for (c = 0; c < num_subdoms; c++) {
for (d = 0; d < cur_subdomains_count; d++) {
- if (strcasecmp(subdomains[c]->name,
+ if (strcasecmp(subdoms[c].name,
cur_subdomains[d]->name) == 0) {
keep_subdomain[d] = true;
/* sub-domain already in cache, nothing to do */
@@ -487,14 +488,14 @@ errno_t sysdb_update_subdomains(struct sysdb_ctx *sysdb,
if (d == cur_subdomains_count) {
DEBUG(SSSDBG_TRACE_FUNC, ("Adding sub-domain [%s].\n",
- subdomains[c]->name));
- ret = sysdb_domain_create(sysdb, subdomains[c]->name);
+ subdoms[c].name));
+ ret = sysdb_domain_create(sysdb, subdoms[c].name);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, ("sysdb_domain_create failed.\n"));
goto done;
}
- ret = sysdb_add_subdomain_attributes(sysdb, subdomains[c]);
+ ret = sysdb_add_subdomain_attributes(sysdb, &subdoms[c]);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
("sysdb_add_subdomain_attributes failed.\n"));
diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index ba43d7d4913642da5ce96f2a7289fb837de3c1ce..0717bff347b2900768c1cbe3233d952ee8f5aa24 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -414,7 +414,6 @@ static int be_get_subdomains(DBusMessage *message, struct sbus_connection *conn)
}
req->force = force;
req->domain_hint = talloc_strdup(req, domain_hint);
- req->domain_list = NULL;
if (!req->domain_hint) {
err_maj = DP_ERR_FATAL;
err_min = ENOMEM;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h
index 23b8f8bfacd33b0a03b3b4f69eb1e8e2848ab751..436765aad0c972c3461c938557b739f7dfbcaf24 100644
--- a/src/providers/dp_backend.h
+++ b/src/providers/dp_backend.h
@@ -173,7 +173,6 @@ struct be_autofs_req {
struct be_subdom_req {
bool force;
char *domain_hint;
- struct sysdb_subdom **domain_list;
};
struct be_host_req {
diff --git a/src/providers/ipa/ipa_subdomains.c b/src/providers/ipa/ipa_subdomains.c
index a8c05c2a744eb8476e363585659c83c12b964b0a..b8d072010fb570210012516944adbacd1e2bb42c 100644
--- a/src/providers/ipa/ipa_subdomains.c
+++ b/src/providers/ipa/ipa_subdomains.c
@@ -61,6 +61,11 @@ struct ipa_subdomains_ctx {
struct sdap_search_base **search_bases;
struct sdap_search_base **master_search_bases;
struct sdap_search_base **ranges_search_bases;
+
+ /* subdomain map cache */
+ time_t last_retrieved;
+ int num_subdoms;
+ struct sysdb_subdom *subdoms;
};
static void ipa_subdomains_reply(struct be_req *be_req, int dp_err, int result)
@@ -175,89 +180,172 @@ static char *name_to_realm(TALLOC_CTX *memctx, const char *name)
return realm;
}
-static errno_t ipa_subdomains_parse_results(struct be_subdom_req *sd_data,
- size_t count,
- struct sysdb_attrs **reply)
+static errno_t ipa_subdom_parse(TALLOC_CTX *memctx,
+ struct sysdb_attrs *attrs,
+ struct sysdb_subdom *subdom)
{
- struct sysdb_subdom **new_domain_list = NULL;
const char *value;
- size_t c;
int ret;
- new_domain_list = talloc_array(sd_data, struct sysdb_subdom *, count + 1);
- if (new_domain_list == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
- return ENOMEM;
+ ret = sysdb_attrs_get_string(attrs, IPA_CN, &value);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_string failed.\n"));
+ return ret;
}
-
- for (c = 0; c < count; c++) {
- new_domain_list[c] = talloc_zero(new_domain_list,
- struct sysdb_subdom);
- if (new_domain_list[c] == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, ("talloc_zero failed.\n"));
- ret = ENOMEM;
- goto done;
- }
-
- ret = sysdb_attrs_get_string(reply[c], IPA_CN, &value);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_string failed.\n"));
- goto done;
- }
- new_domain_list[c]->name = talloc_strdup(new_domain_list[c], value);
- if (new_domain_list[c]->name == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n"));
- ret = ENOMEM;
- goto done;
+ if (subdom->name == NULL) {
+ subdom->name = talloc_strdup(memctx, value);
+ if (subdom->name == NULL) {
+ return ENOMEM;
}
+ } else if (strcmp(subdom->name, value) != 0) {
+ DEBUG(SSSDBG_OP_FAILURE, ("subdomain name mismatch!\n"));
+ return EINVAL;
+ }
+ if (subdom->realm == NULL) {
/* Add Realm as upper(domain name), this is generally always correct
* with AD domains */
- new_domain_list[c]->realm = name_to_realm(new_domain_list[c],
- new_domain_list[c]->name);
- if (!new_domain_list[c]->realm) {
- DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n"));
- ret = ENOMEM;
- goto done;
+ subdom->realm = name_to_realm(memctx, subdom->name);
+ if (!subdom->realm) {
+ return ENOMEM;
+ }
+ }
+
+ ret = sysdb_attrs_get_string(attrs, IPA_FLATNAME, &value);
+ if (ret) {
+ DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_string failed.\n"));
+ return ret;
+ }
+
+ /* in theory this may change, it should never happen, so we will log a
+ * warning if it does, but we will allow it for now */
+ if (subdom->flat_name != NULL) {
+ if (strcmp(subdom->flat_name, value) != 0) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("Flat name for subdomain changed!\n"));
+ talloc_free(discard_const(subdom->flat_name));
+ subdom->flat_name = (const char *)NULL;
+ }
+ }
+ if (subdom->flat_name == NULL) {
+ subdom->flat_name = talloc_strdup(memctx, value);
+ if (subdom->flat_name == NULL) {
+ return ENOMEM;
}
+ }
+
+ ret = sysdb_attrs_get_string(attrs, IPA_TRUSTED_DOMAIN_SID, &value);
+ if (ret) {
+ DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_string failed.\n"));
+ return ret;
+ }
+
+ /* in theory this may change, it should never happen, so we will log a
+ * warning if it does, but we will allow it for now */
+ if (subdom->id != NULL) {
+ if (strcmp(subdom->id, value) != 0) {
+ DEBUG(SSSDBG_TRACE_INTERNAL,
+ ("ID for subdomain changed!\n"));
+ talloc_free(discard_const(subdom->id));
+ subdom->flat_name = (const char *)NULL;
+ }
+ }
+ if (subdom->id == NULL) {
+ subdom->id = talloc_strdup(memctx, value);
+ if (subdom->id == NULL) {
+ return ENOMEM;
+ }
+ }
- ret = sysdb_attrs_get_string(reply[c], IPA_FLATNAME, &value);
- if (ret == EOK) {
- new_domain_list[c]->flat_name = talloc_strdup(new_domain_list[c],
- value);
- if (new_domain_list[c]->flat_name == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n"));
- ret = ENOMEM;
+ return EOK;
+}
+
+static errno_t ipa_subdomains_refresh(struct ipa_subdomains_ctx *ctx,
+ int count, struct sysdb_attrs **reply,
+ bool *changes)
+{
+ bool handled[count];
+ const char *value;
+ int c, h;
+ int ret;
+ int i, j;
+
+ memset(handled, 0, sizeof(bool) * count);
+
+ /* check existing subdoms in cache */
+ for (i = 0, h = 0; i < ctx->num_subdoms; i++) {
+ for (c = 0; c < count; c++) {
+ if (handled[c]) {
+ continue;
+ }
+ ret = sysdb_attrs_get_string(reply[c], IPA_CN, &value);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_string failed.\n"));
goto done;
}
- } else if (ret != ENOENT) {
- DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_string failed.\n"));
- goto done;
+ if (strcmp(value, ctx->subdoms[i].name) == 0) {
+ break;
+ }
}
- ret = sysdb_attrs_get_string(reply[c], IPA_TRUSTED_DOMAIN_SID, &value);
- if (ret == EOK) {
- new_domain_list[c]->id = talloc_strdup(new_domain_list[c], value);
- if (new_domain_list[c]->id == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n"));
- ret = ENOMEM;
+ if (c >= count) {
+ /* ok this subdomain does not exist anymore, let's clean up */
+ for (j = i; j < ctx->num_subdoms - 1; j++) {
+ ctx->subdoms[j] = ctx->subdoms[j + 1];
+ }
+ ctx->num_subdoms--;
+ i--;
+ } else {
+ /* ok let's try to update it */
+ ret = ipa_subdom_parse(ctx->subdoms, reply[c], &ctx->subdoms[i]);
+ if (ret) {
+ DEBUG(SSSDBG_OP_FAILURE, ("Failed to parse subdom data\n"));
goto done;
}
- } else if (ret != ENOENT) {
- DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_get_string failed.\n"));
+ handled[c] = true;
+ h++;
+ }
+ }
+
+ if (count == h) {
+ /* all domains were already accounted for and have been updated */
+ ret = EOK;
+ goto done;
+ }
+
+ /* if we get here it means we have changes to the subdomains list */
+ *changes = true;
+
+ /* add space for unhandled domains */
+ c = count - h;
+ ctx->subdoms = talloc_realloc(ctx, ctx->subdoms,
+ struct sysdb_subdom,
+ ctx->num_subdoms + c);
+ if (ctx->subdoms == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ for (c = 0; c < count; c++) {
+ if (handled[c]) {
+ continue;
+ }
+ i = ctx->num_subdoms;
+ memset(&ctx->subdoms[i], 0, sizeof(struct sysdb_subdom));
+ ret = ipa_subdom_parse(ctx->subdoms, reply[c], &ctx->subdoms[i]);
+ if (ret) {
+ DEBUG(SSSDBG_OP_FAILURE, ("Failed to parse subdom data\n"));
goto done;
}
+ ctx->num_subdoms++;
}
- new_domain_list[c] = NULL;
ret = EOK;
done:
- if (ret == EOK) {
- talloc_free(sd_data->domain_list);
- sd_data->domain_list = new_domain_list;
- } else {
- talloc_free(new_domain_list);
+ if (ret != EOK) {
+ ctx->num_subdoms = 0;
+ talloc_zfree(ctx->subdoms);
}
return ret;
@@ -444,6 +532,7 @@ static void ipa_subdomains_handler_done(struct tevent_req *req)
struct ipa_subdomains_req_ctx *ctx;
struct be_req *be_req;
struct sysdb_ctx *sysdb;
+ bool refresh_has_changes = false;
ctx = tevent_req_callback_data(req, struct ipa_subdomains_req_ctx);
be_req = ctx->be_req;
@@ -480,18 +569,20 @@ static void ipa_subdomains_handler_done(struct tevent_req *req)
goto done;
}
- ret = ipa_subdomains_parse_results(ctx->sd_data, ctx->reply_count,
- ctx->reply);
+ ret = ipa_subdomains_refresh(ctx->sd_ctx, ctx->reply_count, ctx->reply,
+ &refresh_has_changes);
if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE,
- ("ipa_subdomains_parse_results request failed.\n"));
+ DEBUG(SSSDBG_OP_FAILURE, ("Failed to refresh subdomains.\n"));
goto done;
}
- ret = sysdb_update_subdomains(sysdb, ctx->sd_data->domain_list);
- if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, ("sysdb_update_subdomains failed.\n"));
- goto done;
+ if (refresh_has_changes) {
+ ret = sysdb_update_subdomains(sysdb, ctx->sd_ctx->num_subdoms,
+ ctx->sd_ctx->subdoms);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, ("sysdb_update_subdomains failed.\n"));
+ goto done;
+ }
}
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index 1eb9503f0a1df8af2ff18ab4020ed4d9a7810e21..d314baa9579b91fd884a104dc900fc56a590a268 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -3398,9 +3398,10 @@ START_TEST(test_sysdb_subdomain_create)
errno_t ret;
struct sysdb_subdom **cur_subdomains = NULL;
size_t cur_subdomains_count;
- struct sysdb_subdom *new_subdom1[] = { &dom1, NULL};
- struct sysdb_subdom *new_subdom2[] = { &dom2, NULL};
- struct sysdb_subdom *empty[] = { NULL};
+ struct sysdb_subdom *new_subdom1 = &dom1;
+ struct sysdb_subdom *new_subdom2 = &dom2;
+ int num_subdom1 = 1;
+ int num_subdom2 = 1;
ret = setup_sysdb_tests(&test_ctx);
fail_if(ret != EOK, "Could not set up the test");
@@ -3412,7 +3413,7 @@ START_TEST(test_sysdb_subdomain_create)
fail_unless(cur_subdomains != NULL, "No sub-domains returned.");
fail_unless(cur_subdomains[0] == NULL, "No empyt sub-domain list returned.");
- ret = sysdb_update_subdomains(test_ctx->sysdb, new_subdom1);
+ ret = sysdb_update_subdomains(test_ctx->sysdb, num_subdom1, new_subdom1);
fail_unless(ret == EOK, "sysdb_update_subdomains failed with [%d][%s]",
ret, strerror(ret));
@@ -3422,11 +3423,11 @@ START_TEST(test_sysdb_subdomain_create)
ret, strerror(ret));
fail_unless(cur_subdomains != NULL, "No sub-domains returned.");
fail_unless(cur_subdomains[0] != NULL, "Empyt sub-domain list returned.");
- fail_unless(strcmp(cur_subdomains[0]->name, new_subdom1[0]->name) == 0,
+ fail_unless(strcmp(cur_subdomains[0]->name, new_subdom1[0].name) == 0,
"Unexpected sub-domain found, expected [%s], got [%s]",
- new_subdom1[0]->name, cur_subdomains[0]->name);
+ new_subdom1[0].name, cur_subdomains[0]->name);
- ret = sysdb_update_subdomains(test_ctx->sysdb, new_subdom2);
+ ret = sysdb_update_subdomains(test_ctx->sysdb, num_subdom2, new_subdom2);
fail_unless(ret == EOK, "sysdb_update_subdomains failed with [%d][%s]",
ret, strerror(ret));
@@ -3436,11 +3437,11 @@ START_TEST(test_sysdb_subdomain_create)
ret, strerror(ret));
fail_unless(cur_subdomains != NULL, "No sub-domains returned.");
fail_unless(cur_subdomains[0] != NULL, "Empyt sub-domain list returned.");
- fail_unless(strcmp(cur_subdomains[0]->name, new_subdom2[0]->name) == 0,
+ fail_unless(strcmp(cur_subdomains[0]->name, new_subdom2[0].name) == 0,
"Unexpected sub-domain found, expected [%s], got [%s]",
- new_subdom2[0]->name, cur_subdomains[0]->name);
+ new_subdom2[0].name, cur_subdomains[0]->name);
- ret = sysdb_update_subdomains(test_ctx->sysdb, empty);
+ ret = sysdb_update_subdomains(test_ctx->sysdb, 0, NULL);
fail_unless(ret == EOK, "sysdb_update_subdomains failed with [%d][%s]",
ret, strerror(ret));
@@ -3457,7 +3458,8 @@ START_TEST(test_sysdb_subdomain_store_user)
{
struct sysdb_test_ctx *test_ctx;
errno_t ret;
- struct sysdb_subdom *test_subdom[] = { &dom_t, NULL};
+ struct sysdb_subdom *test_subdom = &dom_t;
+ int num_subdom = 1;
struct sss_domain_info *subdomain = NULL;
struct ldb_result *results = NULL;
struct ldb_dn *base_dn = NULL;
@@ -3466,7 +3468,7 @@ START_TEST(test_sysdb_subdomain_store_user)
ret = setup_sysdb_tests(&test_ctx);
fail_if(ret != EOK, "Could not set up the test");
- ret = sysdb_update_subdomains(test_ctx->sysdb, test_subdom);
+ ret = sysdb_update_subdomains(test_ctx->sysdb, num_subdom, test_subdom);
fail_unless(ret == EOK, "sysdb_update_subdomains failed with [%d][%s]",
ret, strerror(ret));
@@ -3512,7 +3514,8 @@ START_TEST(test_sysdb_subdomain_user_ops)
{
struct sysdb_test_ctx *test_ctx;
errno_t ret;
- struct sysdb_subdom *test_subdom[] = { &dom_t, NULL};
+ struct sysdb_subdom *test_subdom = &dom_t;
+ int num_subdom = 1;
struct sss_domain_info *subdomain = NULL;
struct ldb_message *msg = NULL;
struct ldb_dn *check_dn = NULL;
@@ -3520,7 +3523,7 @@ START_TEST(test_sysdb_subdomain_user_ops)
ret = setup_sysdb_tests(&test_ctx);
fail_if(ret != EOK, "Could not set up the test");
- ret = sysdb_update_subdomains(test_ctx->sysdb, test_subdom);
+ ret = sysdb_update_subdomains(test_ctx->sysdb, num_subdom, test_subdom);
fail_unless(ret == EOK, "sysdb_update_subdomains failed with [%d][%s]",
ret, strerror(ret));
@@ -3561,7 +3564,8 @@ START_TEST(test_sysdb_subdomain_group_ops)
{
struct sysdb_test_ctx *test_ctx;
errno_t ret;
- struct sysdb_subdom *test_subdom[] = { &dom_t, NULL};
+ struct sysdb_subdom *test_subdom = &dom_t;
+ int num_subdom = 1;
struct sss_domain_info *subdomain = NULL;
struct ldb_message *msg = NULL;
struct ldb_dn *check_dn = NULL;
@@ -3569,7 +3573,7 @@ START_TEST(test_sysdb_subdomain_group_ops)
ret = setup_sysdb_tests(&test_ctx);
fail_if(ret != EOK, "Could not set up the test");
- ret = sysdb_update_subdomains(test_ctx->sysdb, test_subdom);
+ ret = sysdb_update_subdomains(test_ctx->sysdb, num_subdom, test_subdom);
fail_unless(ret == EOK, "sysdb_update_subdomains failed with [%d][%s]",
ret, strerror(ret));
--
1.7.10.4
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
