On 12/02/2014 12:50 PM, Lukas Slebodnik wrote:
On (29/10/14 17:17), Pavel Reichl wrote:
Hello,
please see attached patch.
This patch is part of solution for
https://fedorahosted.org/sssd/ticket/1991
which aims to unify return values of sysdb calls in case no results are
found.
a) this patch cannot be applied on current master.
rebased
b) I wrote similar patch becuse sssd_be crashed in function
get_object_from_cache. sysdb_search_object_by_sid didn' returned ENOENT and
res->msgs was NULL (result: dereference of NULL pointer)
-- it is yet another result of broken IPA <-> AD trust
c) Could you compare your version and mine?
Feel free to include my patch to yours if it is not good enogh.
I don't say it was not good enough but I updated my patch.
d) test test_sysdb_delete_by_sid seems to be unrelated to this patch.
I would prefer to have it in separete patch.
I have moved it to separate patch. This patch is just to verify that
sysdb_delete_by_sid() was not changed by the second patch.
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks, please see updated patches.
>From 53c3be375eff41dc70e17bb9e079ec98b8c2c5e7 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 9 Dec 2014 09:47:11 +0000
Subject: [PATCH 1/2] TESTS: sysdb_delete_by_sid() test return value
Check that return value of sysdb_delete_by_sid() is not changed as
called SYSDB functions have changed the return value.
Part of patches for:
https://fedorahosted.org/sssd/ticket/1991
---
src/tests/sysdb-tests.c | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index b55901a30feb94cd8199eab096785a02dfc89b9a..d303982647547eefdce7c37ac5b70e1ffbe869ce 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -5127,7 +5127,28 @@ START_TEST(test_sysdb_search_object_by_uuid)
"UUIDuser") == 0, "Unexpected object found, " \
"expected [%s], got [%s].", "UUIDuser",
ldb_msg_find_attr_as_string(res->msgs[0],SYSDB_NAME, ""));
+ talloc_free(test_ctx);
+}
+END_TEST
+START_TEST(test_sysdb_delete_by_sid)
+{
+ errno_t ret;
+ struct sysdb_test_ctx *test_ctx;
+
+ /* Setup */
+ ret = setup_sysdb_tests(&test_ctx);
+ fail_if(ret != EOK, "Could not set up the test");
+
+ check_leaks_push(test_ctx);
+
+ /* Delete the group by SID */
+ ret = sysdb_delete_by_sid(test_ctx->sysdb, test_ctx->domain,
+ "S-1-2-3-4-NON_EXISTING_SID");
+ fail_unless(ret == EOK, "sysdb_delete_by_sid failed with [%d][%s].",
+ ret, strerror(ret));
+
+ fail_unless(check_leaks_pop(test_ctx) == true, "Memory leak");
talloc_free(test_ctx);
}
END_TEST
@@ -6175,6 +6196,7 @@ Suite *create_sysdb_suite(void)
tcase_add_test(tc_sysdb, test_sysdb_search_custom_update);
tcase_add_test(tc_sysdb, test_sysdb_search_custom);
tcase_add_test(tc_sysdb, test_sysdb_delete_custom);
+ tcase_add_test(tc_sysdb, test_sysdb_delete_by_sid);
/* test recursive delete */
tcase_add_test(tc_sysdb, test_sysdb_delete_recursive);
--
1.9.3
>From 1cb395244d3ab3906af5b6c8b294e80055ad0a07 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 9 Dec 2014 11:01:13 +0000
Subject: [PATCH 2/2] SYSDB: sysdb_search_object_by_sid returns ENOENT
sysdb_search_object_by_sid returns ENOENT if no results are found.
Part od solution for:
https://fedorahosted.org/sssd/ticket/1991
---
src/db/sysdb.h | 2 +-
src/db/sysdb_ops.c | 70 ++++++------------------------------------
src/responder/nss/nsssrv_cmd.c | 27 ++++++++--------
src/responder/pac/pacsrv_cmd.c | 26 ++++++++++------
src/tests/sysdb-tests.c | 5 +--
5 files changed, 42 insertions(+), 88 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 5bd7f90acb685bbaff5c98f433c7dce8175c33ca..9b88b4a63619456f8f3cc1961e29bbf3946ca5b8 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -1033,7 +1033,7 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *sid_str,
const char **attrs,
- struct ldb_result **msg);
+ struct ldb_result **res);
errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
index 998046a2ca1c746b2032f430e5f9c4a7151e1dbc..8d6b11c248fd7f895ff6a95c25d4372fc7f8445d 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -2995,8 +2995,15 @@ int sysdb_delete_by_sid(struct sysdb_ctx *sysdb,
ret = sysdb_search_object_by_sid(tmp_ctx, domain, sid_str, NULL, &res);
if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: %d (%s)\n",
- ret, strerror(ret));
+ if (ret == ENOENT) {
+ /* No existing entry. Just quit. */
+ DEBUG(SSSDBG_TRACE_FUNC,
+ "search by sid did not return any results.\n");
+ ret = EOK;
+ } else {
+ DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: %d (%s)\n",
+ ret, strerror(ret));
+ }
goto done;
}
@@ -3007,12 +3014,6 @@ int sysdb_delete_by_sid(struct sysdb_ctx *sysdb,
goto done;
}
- if (res->count == 0) {
- /* No existing entry. Just quit. */
- ret = EOK;
- goto done;
- }
-
ret = sysdb_delete_entry(sysdb, res->msgs[0]->dn, false);
if (ret != EOK) {
goto done;
@@ -3564,61 +3565,10 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *sid_str,
const char **attrs,
- struct ldb_result **msg)
+ struct ldb_result **res)
{
-/* TODO: use
return sysdb_search_object_by_str_attr(mem_ctx, domain, SYSDB_SID_FILTER,
sid_str, attrs, res);
-
- when verified that all callers can handle ENOENT correctly. */
-
- TALLOC_CTX *tmp_ctx;
- const char *def_attrs[] = { SYSDB_NAME, SYSDB_UIDNUM, SYSDB_GIDNUM,
- ORIGINALAD_PREFIX SYSDB_NAME,
- SYSDB_OBJECTCLASS, NULL };
- struct ldb_dn *basedn;
- int ret;
- struct ldb_result *res = NULL;
-
- tmp_ctx = talloc_new(NULL);
- if (!tmp_ctx) {
- return ENOMEM;
- }
-
- basedn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb, SYSDB_DOM_BASE, domain->name);
- if (basedn == NULL) {
- DEBUG(SSSDBG_OP_FAILURE, "ldb_dn_new_fmt failed.\n");
- ret = ENOMEM;
- goto done;
- }
-
- ret = ldb_search(domain->sysdb->ldb, tmp_ctx, &res,
- basedn, LDB_SCOPE_SUBTREE, attrs?attrs:def_attrs,
- SYSDB_SID_FILTER, sid_str);
- if (ret != EOK) {
- ret = sysdb_error_to_errno(ret);
- DEBUG(SSSDBG_OP_FAILURE, "ldb_search failed.\n");
- goto done;
- }
-
- if (res->count > 1) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Search for SID [%s] returned more than " \
- "one object.\n", sid_str);
- ret = EINVAL;
- goto done;
- }
-
- *msg = talloc_steal(mem_ctx, res);
-
-done:
- if (ret == ENOENT) {
- DEBUG(SSSDBG_TRACE_FUNC, "No such entry.\n");
- } else if (ret) {
- DEBUG(SSSDBG_OP_FAILURE, "Error: %d (%s)\n", ret, strerror(ret));
- }
-
- talloc_zfree(tmp_ctx);
- return ret;
}
errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx,
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index ea58920bc3611c84958e8d0aca0e122d90c68e5c..d81737b6182bf283f61655c59dcffa135a1244d7 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4492,6 +4492,20 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
ret = sysdb_search_object_by_sid(cmdctx, dom, cmdctx->secid, NULL,
&dctx->res);
if (ret != EOK) {
+ if (ret == ENOENT) {
+ if (!dctx->check_provider) {
+ DEBUG(SSSDBG_OP_FAILURE, "No results for getbysid call.\n");
+
+ /* set negative cache only if not result of cache check */
+ ret = sss_ncache_set_sid(nctx->ncache, false, cmdctx->secid);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "Cannot set negative cache for %s\n", cmdctx->secid);
+ }
+ }
+ return ENOENT;
+ }
+
DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to our cache!\n");
return EIO;
}
@@ -4502,19 +4516,6 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
return ENOENT;
}
- if (dctx->res->count == 0 && !dctx->check_provider) {
- DEBUG(SSSDBG_OP_FAILURE, "No results for getbysid call.\n");
-
- /* set negative cache only if not result of cache check */
- ret = sss_ncache_set_sid(nctx->ncache, false, cmdctx->secid);
- if (ret != EOK) {
- DEBUG(SSSDBG_MINOR_FAILURE,
- "Cannot set negative cache for %s\n", cmdctx->secid);
- }
-
- return ENOENT;
- }
-
/* if this is a caching provider (or if we haven't checked the cache
* yet) then verify that the cache is uptodate */
if (dctx->check_provider) {
diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
index cc92592893899b1fa269188facc1a8154f80991d..5cedea14747b578dd7896d4d9a80c2e81fc04c17 100644
--- a/src/responder/pac/pacsrv_cmd.c
+++ b/src/responder/pac/pacsrv_cmd.c
@@ -298,16 +298,17 @@ static void pac_lookup_sids_done(struct tevent_req *req)
ret = sysdb_search_object_by_sid(pr_ctx, dom, entries[c].key.str,
NULL, &msg);
if (ret != EOK) {
- DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_object_by_sid " \
- "failed.\n");
+ if (ret == ENOENT) {
+ DEBUG(SSSDBG_OP_FAILURE, "No entry found for SID [%s].\n",
+ entries[c].key.str);
+ } else {
+ DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_object_by_sid " \
+ "failed.\n");
+ }
continue;
}
- if (msg->count == 0) {
- DEBUG(SSSDBG_OP_FAILURE, "No entry found for SID [%s].\n",
- entries[c].key.str);
- continue;
- } else if (msg->count > 1) {
+ if (msg->count > 1) {
DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \
"for SID [%s].\n",
entries[c].key.str);
@@ -912,9 +913,14 @@ pac_store_membership(struct pac_req_ctx *pr_ctx,
ret = sysdb_search_object_by_sid(tmp_ctx, grp_dom, grp_sid_str,
group_attrs, &group);
if (ret != EOK) {
- DEBUG(SSSDBG_TRACE_INTERNAL, "sysdb_search_object_by_sid " \
- "for SID [%s] failed [%d][%s].\n",
- grp_sid_str, ret, strerror(ret));
+ if (ret == ENOENT) {
+ DEBUG(SSSDBG_OP_FAILURE, "Unexpected number of groups returned.\n");
+ ret = EINVAL;
+ } else {
+ DEBUG(SSSDBG_TRACE_INTERNAL, "sysdb_search_object_by_sid " \
+ "for SID [%s] failed [%d][%s].\n",
+ grp_sid_str, ret, strerror(ret));
+ }
goto done;
}
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index d303982647547eefdce7c37ac5b70e1ffbe869ce..92b41e90d08c2d50775289ba8c922f39350ce625 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -4861,13 +4861,10 @@ START_TEST (test_sysdb_search_return_ENOENT)
talloc_zfree(res);
/* Search object */
- /* TODO: Should return ENOENT */
ret = sysdb_search_object_by_sid(test_ctx, test_ctx->domain,
"S-5-4-3-2-1", NULL, &res);
- fail_unless(ret == EOK, "sysdb_search_object_by_sid_str failed with "
+ fail_unless(ret == ENOENT, "sysdb_search_object_by_sid_str failed with "
"[%d][%s].", ret, strerror(ret));
- fail_unless(res->count == 0, "sysdb_search_object_by_sid_str should not "
- "return anything.");
talloc_zfree(res);
/* Search can return more results */
--
1.9.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
