On 12/10/2014 09:28 AM, Lukas Slebodnik wrote:
On (09/12/14 12:13), Pavel Reichl wrote:
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
Thanks, please see updated patches. From 53c3be375eff41dc70e17bb9e079ec98b8c2c5e7 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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
LGTM
From 1cb395244d3ab3906af5b6c8b294e80055ad0a07 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@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;I'm sorry but this isn't very readable. I prefer flat version. If/else if/else is easier to read and if you want to do micro optimisation then you can use switch case.
I find it better readable this way - errors are handled in single block. It's not against any code style rules I'm aware of. But I suppose if this is the only way to get the patches pushed I'll do it your preferred way...
}@@ -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;
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);}}BTW you override error from sss_ncache_set_sid on next line.
But this is exactly same as it was. Do you think it is wrong?
return ENOENT;}Please use flat version of if/else instead of nested one. (or switch case)
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 " \^^ wrapping new line is useless
"failed.\n");} continue; }Please use flat version of if/else instead of nested one. (or switch case)
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 " \^^ wrapping new line is useless
"for SID [%s] failed [%d][%s].\n",grp_sid_str, ret, strerror(ret));} goto done;Please use flat version of if/else instead of nested one. (or switch case)
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));
^^ You touched code around. So you can fix indentation as well.LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel