On Thu, Oct 29, 2009 at 09:15:23AM -0400, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 10/28/2009 02:55 PM, Sumit Bose wrote:
> > Hi,
> >
> > this patch makes the sysdb_search_entry request more flexible by
> > enableing it to return more than one result. I have modified the current
> > callers so that they only take the first result and send a DEBUG message
> > if there are more than one results.
> >
> > bye,
> > Sumit
> >
> >
> >
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel@lists.fedorahosted.org
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> Nack.
>
> In sysdb_search_entry_done() please move state->msgs_count++; above the
> talloc_realloc and use state->msgs_count + 1 for the size instead of +2.
> It reads better (one extra for the NULL). I'd also prefer if you used
> 'if (state->msgs_count == 0)' instead of 'if (!state->msgs)' in
> LDB_REPLY_DONE, because it's technically possible for state->msgs[0] ==
> NULL and this test will still succeed (and be wrong).
>
> In the assorted _recv() functions, you implicitly return the first entry
> found. This is a distinct break from the previous functionality, where
> sysdb_search_entry_done() would have thrown an error if the count was >
> 1. I think we need to continue reporting an error here instead of
> returning possibly incorrect data.* Besides, if we've gotten a
> multiple-value return for a base search, there's something seriously
> wrong and this needs to be reported.
>
> * The data could be incorrect because we don't necessarily know whether
> the LDB will always return the same user first.
>
> - --
> Stephen Gallagher
> RHCE 804006346421761
>
> Delivering value year after year.
> Red Hat ranks #1 in value among software vendors.
> http://www.redhat.com/promo/vendor/
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (GNU/Linux)
> Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/
>
> iEYEARECAAYFAkrplWYACgkQeiVVYja6o6MMcQCfVKSdyReoJ2tNFlSdLesFroqc
> npEAn0iMItMh9/J9O/RjsB0JSPzcjS6S
> =ekUQ
> -----END PGP SIGNATURE-----
ok, the _recv functions return EFAULT now and the
'if (state->msgs_count == 0)' check is used. I haven't changed the
realloc section because it would result in a 'state->msgs_count -1'
in an index.
Thanks for the review.
bye,
Sumit
>From 8c28f0936e02fdaeab6105c3e646aecce1027289 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Wed, 28 Oct 2009 19:42:06 +0100
Subject: [PATCH] Allow sysdb_search_entry request to return more than one result
---
server/db/sysdb.h | 3 +-
server/db/sysdb_ops.c | 90 +++++++++++++++++++++++++----------
server/tests/sysdb-tests.c | 112 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 178 insertions(+), 27 deletions(-)
diff --git a/server/db/sysdb.h b/server/db/sysdb.h
index 5c15d3a..00a3378 100644
--- a/server/db/sysdb.h
+++ b/server/db/sysdb.h
@@ -321,7 +321,8 @@ struct tevent_req *sysdb_search_entry_send(TALLOC_CTX
*mem_ctx,
const char **attrs);
int sysdb_search_entry_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
- struct ldb_message **msg);
+ size_t *msgs_size,
+ struct ldb_message ***msgs);
/* Search User (by uid or name) */
struct tevent_req *sysdb_search_user_by_name_send(TALLOC_CTX *mem_ctx,
diff --git a/server/db/sysdb_ops.c b/server/db/sysdb_ops.c
index 1802234..acff5e5 100644
--- a/server/db/sysdb_ops.c
+++ b/server/db/sysdb_ops.c
@@ -203,6 +203,8 @@ struct sysdb_op_state {
bool ignore_not_found;
struct ldb_reply *ldbreply;
+ size_t msgs_count;
+ struct ldb_message **msgs;
};
static void sysdb_op_default_done(struct tevent_req *subreq)
@@ -322,6 +324,8 @@ struct tevent_req *sysdb_search_entry_send(TALLOC_CTX
*mem_ctx,
state->handle = handle;
state->ignore_not_found = false;
state->ldbreply = NULL;
+ state->msgs_count = 0;
+ state->msgs = NULL;
ret = ldb_build_search_req(&ldbreq, handle->ctx->ldb, state,
base_dn, scope, filter, attrs,
@@ -354,6 +358,7 @@ static void sysdb_search_entry_done(struct tevent_req
*subreq)
struct sysdb_op_state *state = tevent_req_data(req,
struct sysdb_op_state);
struct ldb_reply *ldbreply;
+ struct ldb_message **dummy;
int ret;
ret = sldb_request_recv(subreq, state, &ldbreply);
@@ -365,27 +370,32 @@ static void sysdb_search_entry_done(struct tevent_req
*subreq)
switch (ldbreply->type) {
case LDB_REPLY_ENTRY:
- if (state->ldbreply) {
- DEBUG(1, ("More than one reply for a base search ?! "
- "DB seems corrupted, aborting."));
- tevent_req_error(req, EFAULT);
+ dummy = talloc_realloc(state, state->msgs,
+ struct ldb_message *,
+ state->msgs_count + 2);
+ if (dummy == NULL) {
+ tevent_req_error(req, ENOMEM);
return;
}
+ state->msgs = dummy;
- /* save the entry so that it can be retrieved by the caller */
- state->ldbreply = ldbreply;
+ state->msgs[state->msgs_count + 1] = NULL;
- /* just return, wait for a LDB_REPLY_DONE entry */
+ state->msgs[state->msgs_count] = talloc_steal(state->msgs,
+ ldbreply->message);
+ state->msgs_count++;
+
+ talloc_zfree(ldbreply);
return;
case LDB_REPLY_DONE:
- if (!state->ldbreply) {
- talloc_zfree(ldbreply);
+ talloc_zfree(subreq);
+ talloc_zfree(ldbreply);
+ if (state->msgs_count == 0) {
DEBUG(6, ("Error: Entry not Found!\n"));
tevent_req_error(req, ENOENT);
return;
}
- talloc_zfree(ldbreply);
return tevent_req_done(req);
default:
@@ -399,7 +409,8 @@ static void sysdb_search_entry_done(struct tevent_req
*subreq)
int sysdb_search_entry_recv(struct tevent_req *req,
TALLOC_CTX *mem_ctx,
- struct ldb_message **msg)
+ size_t *msgs_count,
+ struct ldb_message ***msgs)
{
struct sysdb_op_state *state = tevent_req_data(req,
struct sysdb_op_state);
@@ -410,7 +421,8 @@ int sysdb_search_entry_recv(struct tevent_req *req,
return err;
}
- *msg = talloc_move(mem_ctx, &state->ldbreply->message);
+ *msgs_count = state->msgs_count;
+ *msgs = talloc_move(mem_ctx, &state->msgs);
return EOK;
}
@@ -427,7 +439,8 @@ struct sysdb_search_user_state {
const char *filter;
int scope;
- struct ldb_message *msg;
+ size_t msgs_count;
+ struct ldb_message **msgs;
};
static void sysdb_search_user_cont(struct tevent_req *subreq);
@@ -453,7 +466,8 @@ struct tevent_req
*sysdb_search_user_by_name_send(TALLOC_CTX *mem_ctx,
state->ev = ev;
state->handle = handle;
- state->msg = NULL;
+ state->msgs_count = 0;
+ state->msgs = NULL;
state->attrs = attrs ? attrs : def_attrs;
state->filter = NULL;
@@ -512,7 +526,8 @@ struct tevent_req *sysdb_search_user_by_uid_send(TALLOC_CTX
*mem_ctx,
state->ev = ev;
state->handle = handle;
- state->msg = NULL;
+ state->msgs_count = 0;
+ state->msgs = NULL;
state->attrs = attrs ? attrs : def_attrs;
if (!sysdb) sysdb = handle->ctx;
@@ -592,7 +607,8 @@ static void sysdb_search_user_done(struct tevent_req
*subreq)
struct sysdb_search_user_state);
int ret;
- ret = sysdb_search_entry_recv(subreq, state, &state->msg);
+ ret = sysdb_search_entry_recv(subreq, state, &state->msgs_count,
+ &state->msgs);
talloc_zfree(subreq);
if (ret) {
tevent_req_error(req, ret);
@@ -615,7 +631,12 @@ int sysdb_search_user_recv(struct tevent_req *req,
return err;
}
- *msg = talloc_move(mem_ctx, &state->msg);
+ if (state->msgs_count > 1) {
+ DEBUG(1, ("More than one result found.\n"));
+ return EFAULT;
+ }
+
+ *msg = talloc_move(mem_ctx, &state->msgs[0]);
return EOK;
}
@@ -718,7 +739,8 @@ struct sysdb_search_group_state {
const char *filter;
int scope;
- struct ldb_message *msg;
+ size_t msgs_count;
+ struct ldb_message **msgs;
};
static void sysdb_search_group_cont(struct tevent_req *subreq);
@@ -744,7 +766,8 @@ struct tevent_req
*sysdb_search_group_by_name_send(TALLOC_CTX *mem_ctx,
state->ev = ev;
state->handle = handle;
- state->msg = NULL;
+ state->msgs_count = 0;
+ state->msgs = NULL;
state->attrs = attrs ? attrs : def_attrs;
state->filter = NULL;
@@ -803,7 +826,8 @@ struct tevent_req
*sysdb_search_group_by_gid_send(TALLOC_CTX *mem_ctx,
state->ev = ev;
state->handle = handle;
- state->msg = NULL;
+ state->msgs_count = 0;
+ state->msgs = NULL;
state->attrs = attrs ? attrs : def_attrs;
if (!sysdb) sysdb = handle->ctx;
@@ -883,7 +907,8 @@ static void sysdb_search_group_done(struct tevent_req
*subreq)
struct sysdb_search_group_state);
int ret;
- ret = sysdb_search_entry_recv(subreq, state, &state->msg);
+ ret = sysdb_search_entry_recv(subreq, state, &state->msgs_count,
+ &state->msgs);
talloc_zfree(subreq);
if (ret) {
tevent_req_error(req, ret);
@@ -906,7 +931,12 @@ int sysdb_search_group_recv(struct tevent_req *req,
return err;
}
- *msg = talloc_move(mem_ctx, &state->msg);
+ if (state->msgs_count > 1) {
+ DEBUG(1, ("More than one result found.\n"));
+ return EFAULT;
+ }
+
+ *msg = talloc_move(mem_ctx, &state->msgs[0]);
return EOK;
}
@@ -3430,7 +3460,8 @@ struct sysdb_search_custom_state {
const char *filter;
int scope;
- struct ldb_message *msg;
+ size_t msgs_count;
+ struct ldb_message **msgs;
};
static void sysdb_search_custom_check_handle_done(struct tevent_req *subreq);
@@ -3464,7 +3495,8 @@ struct tevent_req
*sysdb_search_custom_by_name_send(TALLOC_CTX *mem_ctx,
state->attrs = attrs;
state->filter = NULL;
state->scope = LDB_SCOPE_BASE;
- state->msg = NULL;
+ state->msgs_count = 0;
+ state->msgs = NULL;
if (sysdb == NULL) {
sysdb = handle->ctx;
@@ -3533,7 +3565,8 @@ static void sysdb_search_custom_done(struct tevent_req
*subreq)
struct sysdb_search_custom_state);
int ret;
- ret = sysdb_search_entry_recv(subreq, state, &state->msg);
+ ret = sysdb_search_entry_recv(subreq, state, &state->msgs_count,
+ &state->msgs);
talloc_zfree(subreq);
if (ret) {
tevent_req_error(req, ret);
@@ -3556,7 +3589,12 @@ int sysdb_search_custom_recv(struct tevent_req *req,
return err;
}
- *msg = talloc_move(mem_ctx, &state->msg);
+ if (state->msgs_count > 1) {
+ DEBUG(1, ("More than one result found.\n"));
+ return EFAULT;
+ }
+
+ *msg = talloc_move(mem_ctx, &state->msgs[0]);
return EOK;
}
diff --git a/server/tests/sysdb-tests.c b/server/tests/sysdb-tests.c
index bb84b2a..edd162c 100644
--- a/server/tests/sysdb-tests.c
+++ b/server/tests/sysdb-tests.c
@@ -163,6 +163,9 @@ struct test_data {
const char *attrval; /* testing sysdb_get_user_attr */
const char **attrlist;
struct ldb_message *msg;
+
+ size_t msgs_count;
+ struct ldb_message **msgs;
};
static int test_loop(struct test_data *data)
@@ -966,6 +969,47 @@ static void test_asq_search_done(struct tevent_req *req)
return;
}
+static void test_search_all_users_done(struct tevent_req *subreq);
+static void test_search_all_users(struct tevent_req *subreq)
+{
+ struct test_data *data = tevent_req_callback_data(subreq,
+ struct test_data);
+ struct ldb_dn *base_dn;
+ int ret;
+
+ ret = sysdb_transaction_recv(subreq, data, &data->handle);
+ talloc_zfree(subreq);
+ if (ret != EOK) {
+ return test_return(data, ret);
+ }
+
+ base_dn = ldb_dn_new_fmt(data, data->ctx->sysdb->ldb, SYSDB_TMPL_USER_BASE,
+ "LOCAL");
+ if (base_dn == NULL) {
+ return test_return(data, ENOMEM);
+ }
+
+ subreq = sysdb_search_entry_send(data, data->ev, data->handle,
+ base_dn, LDB_SCOPE_SUBTREE,
+ "objectClass=user", data->attrlist);
+ if (!subreq) {
+ return test_return(data, ENOMEM);
+ }
+ tevent_req_set_callback(subreq, test_search_all_users_done, data);
+}
+
+static void test_search_all_users_done(struct tevent_req *subreq)
+{
+ struct test_data *data = tevent_req_callback_data(subreq, struct
test_data);
+ int ret;
+
+ ret = sysdb_search_entry_recv(subreq, data, &data->msgs_count,
&data->msgs);
+ talloc_zfree(subreq);
+
+ test_return(data, ret);
+ return;
+}
+
START_TEST (test_sysdb_store_user)
{
struct sysdb_test_ctx *test_ctx;
@@ -2101,6 +2145,71 @@ START_TEST (test_sysdb_asq_search)
}
END_TEST
+START_TEST (test_sysdb_search_all_users)
+{
+ struct sysdb_test_ctx *test_ctx;
+ struct test_data *data;
+ struct tevent_req *req;
+ int ret;
+ int i;
+ char *uid_str;
+
+ /* Setup */
+ ret = setup_sysdb_tests(&test_ctx);
+ if (ret != EOK) {
+ fail("Could not set up the test");
+ return;
+ }
+
+ data = talloc_zero(test_ctx, struct test_data);
+ data->ctx = test_ctx;
+ data->ev = test_ctx->ev;
+ data->attrlist = talloc_array(data, const char *, 2);
+ fail_unless(data->attrlist != NULL, "talloc_array failed");
+
+ data->attrlist[0] = "uidNumber";
+ data->attrlist[1] = NULL;
+
+ req = sysdb_transaction_send(data, data->ev, test_ctx->sysdb);
+ if (!req) {
+ ret = ENOMEM;
+ }
+
+ if (ret == EOK) {
+ tevent_req_set_callback(req, test_search_all_users, data);
+
+ ret = test_loop(data);
+ }
+
+ fail_if(ret != EOK, "Search failed");
+
+ fail_unless(data->msgs_count == 10,
+ "wrong number of results, found [%d] expected [10]",
+ data->msgs_count);
+
+ for (i = 0; i < data->msgs_count; i++) {
+ fail_unless(data->msgs[i]->num_elements == 1,
+ "wrong number of elements, found [%d] expected [1]",
+ data->msgs[i]->num_elements);
+
+ fail_unless(data->msgs[i]->elements[0].num_values == 1,
+ "wrong number of values, found [%d] expected [1]",
+ data->msgs[i]->elements[0].num_values);
+
+ uid_str = talloc_asprintf(data, "%d", 27010 + i);
+ fail_unless(uid_str != NULL, "talloc_asprintf failed.");
+ fail_unless(strncmp(uid_str,
+ (char *) data->msgs[i]->elements[0].values[0].data,
+ data->msgs[i]->elements[0].values[0].length) == 0,
+ "wrong value, found [%.*s] expected [%s]",
+ data->msgs[i]->elements[0].values[0].length,
+ data->msgs[i]->elements[0].values[0].data,
uid_str);
+ }
+
+ talloc_free(test_ctx);
+}
+END_TEST
+
Suite *create_sysdb_suite(void)
{
Suite *s = suite_create("sysdb");
@@ -2167,6 +2276,9 @@ Suite *create_sysdb_suite(void)
tcase_add_loop_test(tc_sysdb, test_sysdb_prepare_asq_test_user, 28011,
28020);
tcase_add_test(tc_sysdb, test_sysdb_asq_search);
+ /* Test search with more than one result */
+ tcase_add_test(tc_sysdb, test_sysdb_search_all_users);
+
/* Remove the members from the groups */
tcase_add_loop_test(tc_sysdb, test_sysdb_remove_group_member, 28010,
28020);
--
1.6.2.5
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
