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