On 12/15/2014 12:15 PM, Pavel Reichl wrote:
On 12/15/2014 10:12 AM, Lukas Slebodnik wrote:
On (12/12/14 17:10), Pavel Reichl wrote:
On 12/12/2014 04:10 PM, Lukas Slebodnik wrote:
On (12/12/14 16:04), Pavel Reichl wrote:
On 12/12/2014 03:52 PM, Lukas Slebodnik wrote:
On (12/12/14 15:25), Pavel Reichl wrote:
On 12/12/2014 03:02 PM, Lukas Slebodnik wrote:
On (12/12/14 14:25), Pavel Reichl wrote:
On 12/11/2014 06:35 PM, Michal Židek wrote:
I think that smaller nesting levels are better.
In general I would do
if (ret == SPECIFIC_CODE) {
specific_error_handle_block
} else (ret != EOK) {
general_error_handle_block
}
If there are many specific error codes that need to be
addressed,
use switch, but 1-2 else-ifs are ok to me.
This pattern can be used if common_to_all_erros_action exists
(and is
something more than just goto done):
if (ret != EOK) {
if (ret == SPECIFIC_CODE) {
specific_error_handle_block
}
common_to_all_errors_action;
}
So I would slightly prefer Lukas's solution over Pavel's.
In this particullar case I would do:
if (ret == ENOENT) {
....
} else if (ret != EOK) {
....
}
Michal
OK, thank you both for opinions and for comments.
Please see updated patch.
Thanks!
Two questions/comments are inline.
>From 94eade7b3c0c0729f5d4a322061c7c9688bd8130 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
Fixes:
https://fedorahosted.org/sssd/ticket/2520
---
src/db/sysdb.h | 2 +-
src/db/sysdb_ops.c | 68
++++++------------------------------------
src/responder/nss/nsssrv_cmd.c | 27 ++++++++---------
src/responder/pac/pacsrv_cmd.c | 29 ++++++++++--------
src/tests/sysdb-tests.c | 5 +---
5 files changed, 40 insertions(+), 91 deletions(-)
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..3c5d450714fb3f7655cd32aeef900b4f5e9782c7
100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4491,7 +4491,19 @@ 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;
You changed behaviour with this nesting if statements.
ENOENT will be returned every time. Previously it was returned
just in case of
true condition (dctx->res->count == 0 && !dctx->check_provider)
Could you explain such change?
Because otherwise 'res' is accessed in case of ENOENT which
implies segfault
in nss_cmd_getbysid_send_reply() or even earlier in
check_cache(), if I read
the code correctly.
Thank you for explanation.
yw
But it does not change the fact that you change
the behaviour with this refactoring patch.
Please do such change in separate patch.
I don't think this a good idea. The change in sysdb is changing
behaviour -
no doubt about it and I really think I have to amend the code that
uses it.
It is IMO a very bad idea send separate patch that introduces
probable
segfault and fix it in another.
I disagree. The segfault could be even with old code or did I miss
something?
I don't see how it could happen with old code, but if it did - I'll
send the
separate patch as you wish. Otherwise I would prefer to stick with
my current
patch.
How it could segfault?
Firstly, I would like to apologize that I didn't spend lot of time with
a review. I just read the patch
Let's summarise some facts.
* your match is named: sysdb_search_object_by_sid returns ENOENT
sysdb_search_object_by_sid returns ENOENT if no results are found
* It isn't possible to crash with current master.
* You conceal a reason why there could be crash.
The function nss_cmd_getbysid_send_reply checks the result count
for 0,
which we wanted to fix .
* You had to change behaviour of the function nss_cmd_getbysid_search
because there could be crash.
It should be sign for you that something stinks in the code.
Let's imagine situation. We have function1 which is called be
function2 which
ic called by function3 which id called by function4. All these
functions rely
on fact that res->count is 0 they stop execution. You decided to convert
function1 to return ENOENT instead of checking res->count to zero.
All other
functions should be converted as well because res needn't be
initialized and
they could crash. Such change should not be done in single patch. It
would
easily become mess a nightmare for reviewer. There is high
probability of
overlooking some places which was not converted properly. The right
approach is
to convert function from top to down (and not from down to top).
Each patch will contain conversion of just one function:
patch1: function4 returns ENOENT
patch2: function3 returns ENOENT
..
Each patch will be simple and will not change the behaviour of other
function.
I hope it is clear to you now.
Let me know whether you will sent patch ASAP or I should send temporary
solution for ticket #2520
I think it will be best if you send temporally solution. Acking
process for this patch is really cumbersome. :-(
Summary: your solution is partially done therefore NACK
LS
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Updated patches attached.
>From acbcd752ab983f7e286ea84663885499ae03dc86 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/3] 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 a4a7cf3a161d0628d6b84ea925ef86eaecae4d89 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Wed, 17 Dec 2014 14:10:45 +0000
Subject: [PATCH 2/3] NSS: nss_cmd_getbysid_search return ENOENT
---
src/responder/nss/nsssrv_cmd.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/responder/nss/nsssrv_cmd.c b/src/responder/nss/nsssrv_cmd.c
index ea58920bc3611c84958e8d0aca0e122d90c68e5c..80ac221e288665741d8b1e2bd020ecca568106c1 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4502,16 +4502,16 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
return ENOENT;
}
- if (dctx->res->count == 0 && !dctx->check_provider) {
+ if (dctx->res->count == 0) {
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);
+ if (!dctx->check_provider) {
+ /* 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;
}
--
1.9.3
>From e268af4605e8a20a35c667f3681841eae3f5aca0 Mon Sep 17 00:00:00 2001
From: Pavel Reichl <prei...@redhat.com>
Date: Tue, 9 Dec 2014 11:01:13 +0000
Subject: [PATCH 3/3] 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
Fixes:
https://fedorahosted.org/sssd/ticket/2520
---
src/db/sysdb.h | 2 +-
src/db/sysdb_ops.c | 68 ++++++------------------------------------
src/responder/nss/nsssrv_cmd.c | 25 ++++++++--------
src/responder/pac/pacsrv_cmd.c | 29 ++++++++++--------
src/tests/sysdb-tests.c | 5 +---
5 files changed, 39 insertions(+), 90 deletions(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 01900425ac5e8733eb57877bbadef0e8da00475f..b1e057107cc6e3d4ce7b7bb8e821a2414c3424a7 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -1035,7 +1035,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 768f9455329b136b3de9794ad127dd349f1eaa43..b12540b68d1c81c419455416294f3449dd84914e 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -2994,7 +2994,14 @@ 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) {
+
+ if (ret == ENOENT) {
+ /* No existing entry. Just quit. */
+ DEBUG(SSSDBG_TRACE_FUNC,
+ "search by sid did not return any results.\n");
+ ret = EOK;
+ goto done;
+ } else if (ret != EOK) {
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 80ac221e288665741d8b1e2bd020ecca568106c1..3c5d450714fb3f7655cd32aeef900b4f5e9782c7 100644
--- a/src/responder/nss/nsssrv_cmd.c
+++ b/src/responder/nss/nsssrv_cmd.c
@@ -4491,20 +4491,10 @@ 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) {
- DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to our cache!\n");
- return EIO;
- }
-
- if (dctx->res->count > 1) {
- DEBUG(SSSDBG_FATAL_FAILURE, "getbysid call returned more than one " \
- "result !?!\n");
- return ENOENT;
- }
-
- if (dctx->res->count == 0) {
- DEBUG(SSSDBG_OP_FAILURE, "No results for getbysid call.\n");
+ 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) {
@@ -4513,6 +4503,15 @@ static errno_t nss_cmd_getbysid_search(struct nss_dom_ctx *dctx)
}
}
return ENOENT;
+ } else if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to our cache!\n");
+ return EIO;
+ }
+
+ if (dctx->res->count > 1) {
+ DEBUG(SSSDBG_FATAL_FAILURE, "getbysid call returned more than one " \
+ "result !?!\n");
+ return ENOENT;
}
/* if this is a caching provider (or if we haven't checked the cache
diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
index cc92592893899b1fa269188facc1a8154f80991d..07d2f0cf79b70429dc7cf2784a8e31d651e5095f 100644
--- a/src/responder/pac/pacsrv_cmd.c
+++ b/src/responder/pac/pacsrv_cmd.c
@@ -297,17 +297,17 @@ static void pac_lookup_sids_done(struct tevent_req *req)
msg = NULL;
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");
- continue;
- }
-
- if (msg->count == 0) {
+ if (ret == ENOENT) {
DEBUG(SSSDBG_OP_FAILURE, "No entry found for SID [%s].\n",
- entries[c].key.str);
+ entries[c].key.str);
continue;
- } else if (msg->count > 1) {
+ } else if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ "sysdb_search_object_by_sid failed.\n");
+ continue;
+ }
+
+ if (msg->count > 1) {
DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \
"for SID [%s].\n",
entries[c].key.str);
@@ -911,10 +911,13 @@ 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");
+ goto done;
+ } else 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));
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
