On 08/13/2012 11:13 AM, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 07:12:38PM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 07:09:13PM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 05:13:48PM +0200, Michal Zidek wrote:
This should fix the duplicate detection. Patch is attached. Please
review with caution, i was in hurry.
https://fedorahosted.org/sssd/ticket/1472
Thanks
Michal
I think the general approach with compare function is good, but it
should be a property of the fo_service. I don't think there is any
scenario where we'd like to treat two servers with same user_data
contents as the same.
^^^ Sorry, this should have read "two servers from the same service".
You also need to amend the testsuite as well when changing the fo_
interface, otherwise the test suite doesn't work:
CC src/resolv/fail_over_tests-async_resolv.o
src/tests/fail_over-tests.c: In function ‘test_fo_resolve_service’:
src/tests/fail_over-tests.c:233:5: warning: passing argument 5 of
‘fo_add_server’ makes pointer from integer without a cast [enabled by default]
In file included from ./src/providers/dp_backend.h:26:0,
from ./src/providers/ldap/sdap.h:25,
from ./src/tests/common.h:31,
from src/tests/fail_over-tests.c:35:
./src/providers/fail_over.h:120:5: note: expected ‘int (*)(void *, void *)’ but
argument is of type ‘int’
src/tests/fail_over-tests.c:233:5: error: too few arguments to function
‘fo_add_server’
Ok. I rewrote it, so the user_data_cmp is now part of fo_service.
If it is set to NULL, user_data is not used as part of duplicity detection.
Also testsuite is now updated.
New patch is attached.
Thanks
Michal
>From abeafd14c684dd7d4fa61c2bdf05eb2451f332df Mon Sep 17 00:00:00 2001
From: Michal Zidek <mzi...@redhat.com>
Date: Mon, 13 Aug 2012 16:37:13 +0200
Subject: [PATCH] Duplicate detection in fail over did not work.
https://fedorahosted.org/sssd/ticket/1472
---
src/providers/ad/ad_common.c | 3 ++-
src/providers/data_provider_fo.c | 6 ++++--
src/providers/dp_backend.h | 3 ++-
src/providers/fail_over.c | 30 +++++++++++++++++++++++++++---
src/providers/fail_over.h | 1 +
src/providers/ipa/ipa_common.c | 2 +-
src/providers/krb5/krb5_common.c | 2 +-
src/providers/ldap/ldap_common.c | 2 +-
src/tests/fail_over-tests.c | 10 +++++-----
9 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 800ef13..5473d6a 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -240,7 +240,8 @@ ad_failover_init(TALLOC_CTX *mem_ctx, struct be_ctx *bectx,
goto done;
}
- ret = be_fo_add_service(bectx, AD_SERVICE_NAME);
+ ret = be_fo_add_service(bectx, AD_SERVICE_NAME,
+ (int (*)(void*, void*)) strcmp);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to create failover service!\n"));
goto done;
diff --git a/src/providers/data_provider_fo.c b/src/providers/data_provider_fo.c
index 1c03e31..d005e92 100644
--- a/src/providers/data_provider_fo.c
+++ b/src/providers/data_provider_fo.c
@@ -168,7 +168,8 @@ static struct be_svc_data *be_fo_find_svc_data(struct be_ctx *ctx,
return 0;
}
-int be_fo_add_service(struct be_ctx *ctx, const char *service_name)
+int be_fo_add_service(struct be_ctx *ctx, const char *service_name,
+ int (*user_data_cmp)(void*, void*))
{
struct fo_service *service;
struct be_svc_data *svc;
@@ -185,7 +186,8 @@ int be_fo_add_service(struct be_ctx *ctx, const char *service_name)
/* if not in the be service list, try to create new one */
- ret = fo_new_service(ctx->be_fo->fo_ctx, service_name, &service);
+ ret = fo_new_service(ctx->be_fo->fo_ctx, service_name, user_data_cmp,
+ &service);
if (ret != EOK && ret != EEXIST) {
DEBUG(1, ("Failed to create failover service!\n"));
return ret;
diff --git a/src/providers/dp_backend.h b/src/providers/dp_backend.h
index 4d079c0..7c2101e 100644
--- a/src/providers/dp_backend.h
+++ b/src/providers/dp_backend.h
@@ -218,7 +218,8 @@ typedef void (be_svc_callback_fn_t)(void *, struct fo_server *);
int be_init_failover(struct be_ctx *ctx);
int be_fo_is_srv_identifier(const char *server);
-int be_fo_add_service(struct be_ctx *ctx, const char *service_name);
+int be_fo_add_service(struct be_ctx *ctx, const char *service_name,
+ int (*user_data_cmp) (void*, void*));
int be_fo_service_add_callback(TALLOC_CTX *memctx,
struct be_ctx *ctx, const char *service_name,
be_svc_callback_fn_t *fn, void *private_data);
diff --git a/src/providers/fail_over.c b/src/providers/fail_over.c
index facc990..c861d0f 100644
--- a/src/providers/fail_over.c
+++ b/src/providers/fail_over.c
@@ -66,6 +66,12 @@ struct fo_service {
struct fo_server *active_server;
struct fo_server *last_tried_server;
struct fo_server *server_list;
+
+ /* Function pointed by user_data_cmp returns 0 if user_data is equal
+ * or nonzero value if not. Set to NULL if no user data comparison
+ * is needed in fail over duplicate servers detection.
+ */
+ int (*user_data_cmp)(void*, void*);
};
struct fo_server {
@@ -393,6 +399,7 @@ service_destructor(struct fo_service *service)
int
fo_new_service(struct fo_ctx *ctx, const char *name,
+ int (*user_data_cmp)(void*, void*),
struct fo_service **_service)
{
struct fo_service *service;
@@ -420,6 +427,8 @@ fo_new_service(struct fo_ctx *ctx, const char *name,
return ENOMEM;
}
+ service->user_data_cmp = user_data_cmp;
+
service->ctx = ctx;
DLIST_ADD(ctx->service_list, service);
@@ -520,8 +529,14 @@ fo_add_srv_server(struct fo_service *service, const char *srv,
service->name, proto));
DLIST_FOR_EACH(server, service->server_list) {
- if (server->user_data != user_data)
- continue;
+ /* Compare user data only if user_data_cmp and both arguments
+ * are not NULL.
+ */
+ if (server->service->user_data_cmp) {
+ if (server->service->user_data_cmp(server->user_data, user_data)) {
+ continue;
+ }
+ }
if (fo_is_srv_lookup(server)) {
if (((discovery_domain == NULL &&
@@ -629,10 +644,19 @@ static bool fo_server_match(struct fo_server *server,
int port,
void *user_data)
{
- if (server->port != port || server->user_data != user_data) {
+ if (server->port != port) {
return false;
}
+ /* Compare user data only if user_data_cmp and both arguments
+ * are not NULL.
+ */
+ if (server->service->user_data_cmp && server->user_data && user_data) {
+ if (server->service->user_data_cmp(server->user_data, user_data)) {
+ return false;
+ }
+ }
+
if (name == NULL && server->common == NULL) {
return true;
}
diff --git a/src/providers/fail_over.h b/src/providers/fail_over.h
index b69e8a5..449c125 100644
--- a/src/providers/fail_over.h
+++ b/src/providers/fail_over.h
@@ -96,6 +96,7 @@ struct fo_ctx *fo_context_init(TALLOC_CTX *mem_ctx,
*/
int fo_new_service(struct fo_ctx *ctx,
const char *name,
+ int (*user_data_cmp)(void*, void*),
struct fo_service **_service);
/*
diff --git a/src/providers/ipa/ipa_common.c b/src/providers/ipa/ipa_common.c
index bf62fcb..1c619a8 100644
--- a/src/providers/ipa/ipa_common.c
+++ b/src/providers/ipa/ipa_common.c
@@ -893,7 +893,7 @@ int ipa_service_init(TALLOC_CTX *memctx, struct be_ctx *ctx,
goto done;
}
- ret = be_fo_add_service(ctx, "IPA");
+ ret = be_fo_add_service(ctx, "IPA", (int (*)(void*, void*)) strcmp);
if (ret != EOK) {
DEBUG(1, ("Failed to create failover service!\n"));
goto done;
diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c
index 98a2f7d..f4c0097 100644
--- a/src/providers/krb5/krb5_common.c
+++ b/src/providers/krb5/krb5_common.c
@@ -600,7 +600,7 @@ int krb5_service_init(TALLOC_CTX *memctx, struct be_ctx *ctx,
goto done;
}
- ret = be_fo_add_service(ctx, service_name);
+ ret = be_fo_add_service(ctx, service_name, (int (*)(void*, void*)) strcmp);
if (ret != EOK) {
DEBUG(1, ("Failed to create failover service!\n"));
goto done;
diff --git a/src/providers/ldap/ldap_common.c b/src/providers/ldap/ldap_common.c
index 570ec97..1fa3129 100644
--- a/src/providers/ldap/ldap_common.c
+++ b/src/providers/ldap/ldap_common.c
@@ -1218,7 +1218,7 @@ int sdap_service_init(TALLOC_CTX *memctx, struct be_ctx *ctx,
goto done;
}
- ret = be_fo_add_service(ctx, service_name);
+ ret = be_fo_add_service(ctx, service_name, (int (*)(void*, void*)) strcmp);
if (ret != EOK) {
DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to create failover service!\n"));
goto done;
diff --git a/src/tests/fail_over-tests.c b/src/tests/fail_over-tests.c
index 6f4843c..3a7c4ae 100644
--- a/src/tests/fail_over-tests.c
+++ b/src/tests/fail_over-tests.c
@@ -114,11 +114,11 @@ START_TEST(test_fo_new_service)
sprintf(buf, "service_%d", i);
check_leaks_push(ctx);
- ret = fo_new_service(ctx->fo_ctx, buf, &services[i]);
+ ret = fo_new_service(ctx->fo_ctx, buf, NULL, &services[i]);
fail_if(ret != EOK);
}
- ret = fo_new_service(ctx->fo_ctx, "service_3", &service);
+ ret = fo_new_service(ctx->fo_ctx, "service_3", NULL, &service);
fail_if(ret != EEXIST);
for (i = 9; i >= 0; i--) {
@@ -223,11 +223,11 @@ START_TEST(test_fo_resolve_service)
fail_if(ctx == NULL);
/* Add service. */
- fail_if(fo_new_service(ctx->fo_ctx, "http", &service[0]) != EOK);
+ fail_if(fo_new_service(ctx->fo_ctx, "http", NULL, &service[0]) != EOK);
- fail_if(fo_new_service(ctx->fo_ctx, "ldap", &service[1]) != EOK);
+ fail_if(fo_new_service(ctx->fo_ctx, "ldap", NULL, &service[1]) != EOK);
- fail_if(fo_new_service(ctx->fo_ctx, "ntp", &service[2]) != EOK);
+ fail_if(fo_new_service(ctx->fo_ctx, "ntp", NULL, &service[2]) != EOK);
/* Add servers. */
fail_if(fo_add_server(service[0], "localhost", 20, NULL, true) != EOK);
--
1.7.11.2
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
