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.
Thanks!
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. 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.
d) test test_sysdb_delete_by_sid seems to be unrelated to this patch. I would prefer to have it in separete patch.
LS
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. 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.
d) test test_sysdb_delete_by_sid seems to be unrelated to this patch. I would prefer to have it in separete patch.
LS
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Please send you patch in new thread and then I'll rebase my patches on top of it. Please also fix commit message.
Thanks!
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.
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.
}@@ -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.
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
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
On (10/12/14 10:09), Pavel Reichl wrote:
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...
The problem is that I need to spend a lot of time with "parsing this code" Someone can overlook goto done in this case and introduce a bug. Your solution isn't bad but it isn't straightforward.
This is especially problem in pac responder. There are 3 nested levels of if. WHY? I'm sorry but I can lost easily there.
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; }
//snip
@@ -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;
^^^^^^^^^^^^^ I found another interesting part. Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) was not special cased before your change.
BTW please add ticket https://fedorahosted.org/sssd/ticket/2520 to commit message because this patch fixes the crash.
LS
On 12/10/2014 10:25 AM, Lukas Slebodnik wrote:
On (10/12/14 10:09), Pavel Reichl wrote:
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...
The problem is that I need to spend a lot of time with "parsing this code" Someone can overlook goto done in this case and introduce a bug. Your solution isn't bad but it isn't straightforward.
This is especially problem in pac responder. There are 3 nested levels of if. WHY? I'm sorry but I can lost easily there.
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; }//snip
@@ -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;^^^^^^^^^^^^^ I found another interesting part.Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) was not special cased before your change.
Please again see context: Before changing sysdb_search_object_by_sid() to return ENOENT on no result EOK was returned and condition '(group->count != 1)' was true in that case. So I just kept this behaviour. Is that clear now?
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; }
if (group->count != 1) { DEBUG(SSSDBG_OP_FAILURE, "Unexpected number of groups returned.\n"); ret = EINVAL; goto done; }
BTW please add ticket https://fedorahosted.org/sssd/ticket/2520 to commit message because this patch fixes the crash.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (10/12/14 11:31), Pavel Reichl wrote:
On 12/10/2014 10:25 AM, Lukas Slebodnik wrote:
On (10/12/14 10:09), Pavel Reichl wrote:
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...
The problem is that I need to spend a lot of time with "parsing this code" Someone can overlook goto done in this case and introduce a bug. Your solution isn't bad but it isn't straightforward.
This is especially problem in pac responder. There are 3 nested levels of if. WHY? I'm sorry but I can lost easily there.
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; }//snip
@@ -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;^^^^^^^^^^^^^ I found another interesting part.Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) was not special cased before your change.
Please again see context: Before changing sysdb_search_object_by_sid() to return ENOENT on no result EOK was returned and condition '(group->count != 1)' was true in that case. So I just kept this behaviour. Is that clear now?
Ahh I see. I overlook != 1 I was looking for == 0.
but still it would be better to return ENOENT. So we can distinguish between group->count == 0 and group->count > 1. It should not happen very.
LS
On 12/10/2014 02:30 PM, Lukas Slebodnik wrote:
On (10/12/14 11:31), Pavel Reichl wrote:
On 12/10/2014 10:25 AM, Lukas Slebodnik wrote:
On (10/12/14 10:09), Pavel Reichl wrote:
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...
The problem is that I need to spend a lot of time with "parsing this code" Someone can overlook goto done in this case and introduce a bug. Your solution isn't bad but it isn't straightforward.
This is especially problem in pac responder. There are 3 nested levels of if. WHY? I'm sorry but I can lost easily there.
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; }//snip
@@ -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;^^^^^^^^^^^^^ I found another interesting part.Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) was not special cased before your change.
Please again see context: Before changing sysdb_search_object_by_sid() to return ENOENT on no result EOK was returned and condition '(group->count != 1)' was true in that case. So I just kept this behaviour. Is that clear now?
Ahh I see. I overlook != 1 I was looking for == 0.
but still it would be better to return ENOENT. So we can distinguish between group->count == 0 and group->count > 1. It should not happen very.
LS
hmm sounds right, I just check the calling side...and then update the patch as you proposed.
sssd-devel mailing list
Michal would you be so kind to arbitrate mine and Lukas' little dispute about the nesting ifs? Just say what would you prefer.
Thanks!
sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (10/12/14 14:48), Pavel Reichl wrote:
On 12/10/2014 02:30 PM, Lukas Slebodnik wrote:
On (10/12/14 11:31), Pavel Reichl wrote:
On 12/10/2014 10:25 AM, Lukas Slebodnik wrote:
On (10/12/14 10:09), Pavel Reichl wrote:
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...
The problem is that I need to spend a lot of time with "parsing this code" Someone can overlook goto done in this case and introduce a bug. Your solution isn't bad but it isn't straightforward.
This is especially problem in pac responder. There are 3 nested levels of if. WHY? I'm sorry but I can lost easily there.
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; }//snip
>@@ -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;
^^^^^^^^^^^^^ I found another interesting part.Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) was not special cased before your change.
Please again see context: Before changing sysdb_search_object_by_sid() to return ENOENT on no result EOK was returned and condition '(group->count != 1)' was true in that case. So I just kept this behaviour. Is that clear now?
Ahh I see. I overlook != 1 I was looking for == 0.
but still it would be better to return ENOENT. So we can distinguish between group->count == 0 and group->count > 1. It should not happen very.
LS
hmm sounds right, I just check the calling side...and then update the patch as you proposed.
Michal would you be so kind to arbitrate mine and Lukas' little dispute about the nesting ifs? Just say what would you prefer.
+1 for different opinions.
LS
On 12/10/2014 03:24 PM, Lukas Slebodnik wrote:
On (10/12/14 14:48), Pavel Reichl wrote:
On 12/10/2014 02:30 PM, Lukas Slebodnik wrote:
On (10/12/14 11:31), Pavel Reichl wrote:
On 12/10/2014 10:25 AM, Lukas Slebodnik wrote:
On (10/12/14 10:09), Pavel Reichl wrote:
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...
The problem is that I need to spend a lot of time with "parsing this code" Someone can overlook goto done in this case and introduce a bug. Your solution isn't bad but it isn't straightforward.
This is especially problem in pac responder. There are 3 nested levels of if. WHY? I'm sorry but I can lost easily there.
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; }//snip
>> @@ -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;
^^^^^^^^^^^^^ I found another interesting part.Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) was not special cased before your change.
Please again see context: Before changing sysdb_search_object_by_sid() to return ENOENT on no result EOK was returned and condition '(group->count != 1)' was true in that case. So I just kept this behaviour. Is that clear now?
Ahh I see. I overlook != 1 I was looking for == 0.
but still it would be better to return ENOENT. So we can distinguish between group->count == 0 and group->count > 1. It should not happen very.
LS
hmm sounds right, I just check the calling side...and then update the patch as you proposed.
Michal would you be so kind to arbitrate mine and Lukas' little dispute about the nesting ifs? Just say what would you prefer.
+1 for different opinions.
LS
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
On 12/11/2014 06:35 PM, Michal Židek wrote:
On 12/10/2014 03:24 PM, Lukas Slebodnik wrote:
On (10/12/14 14:48), Pavel Reichl wrote:
On 12/10/2014 02:30 PM, Lukas Slebodnik wrote:
On (10/12/14 11:31), Pavel Reichl wrote:
On 12/10/2014 10:25 AM, Lukas Slebodnik wrote:
On (10/12/14 10:09), Pavel Reichl wrote: > 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... The problem is that I need to spend a lot of time with "parsing this code" Someone can overlook goto done in this case and introduce a bug. Your solution isn't bad but it isn't straightforward.
This is especially problem in pac responder. There are 3 nested levels of if. WHY? I'm sorry but I can lost easily there.
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 ofcache 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; }
//snip
>>> @@ -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; ^^^^^^^^^^^^^ I found another interesting part. Why do you override ENOENT with EINVAL. ENOENT (msg->count == 0) was not special cased before your change.
Please again see context: Before changing sysdb_search_object_by_sid() to return ENOENT on no result EOK was returned and condition '(group->count != 1)' was true in that case. So I just kept this behaviour. Is that clear now?
Ahh I see. I overlook != 1 I was looking for == 0.
but still it would be better to return ENOENT. So we can distinguish between group->count == 0 and group->count > 1. It should not happen very.
LS
hmm sounds right, I just check the calling side...and then update the patch as you proposed.
Michal would you be so kind to arbitrate mine and Lukas' little dispute about the nesting ifs? Just say what would you prefer.
+1 for different opinions.
LS
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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
OK, thank you both for opinions and for comments.
Please see updated patch.
Thanks!
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 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
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?
- } else if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to our cache!\n"); return EIO; }
@@ -4502,19 +4514,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;- }
^^^^ @see removed code.
/* 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..07d2f0cf79b70429dc7cf2784a8e31d651e5095f 100644 --- a/src/responder/pac/pacsrv_cmd.c +++ b/src/responder/pac/pacsrv_cmd.c @@ -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");
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This message can be little bit confusing. We exactly know what's wrong. There isn't such object for SID. It's better to be clear.
LS
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 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
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.
- } else if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to make request to our cache!\n"); return EIO; }
@@ -4502,19 +4514,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;- }
^^^^ @see removed code.
/* 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..07d2f0cf79b70429dc7cf2784a8e31d651e5095f 100644 --- a/src/responder/pac/pacsrv_cmd.c +++ b/src/responder/pac/pacsrv_cmd.c @@ -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");^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This message can be little bit confusing. We exactly know what's wrong. There isn't such object for SID. It's better to be clear.LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 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
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.
But it does not change the fact that you change the behaviour with this refactoring patch. Please do such change in separate patch.
LS
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 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
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.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 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
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? Please split it into separate patch. This change is not related to your ticket and it's better to have ticket per issue.
LS
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 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
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?
Please split it into separate patch. This change is not related to your ticket and it's better to have ticket per issue.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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 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 > >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
Summary: your solution is partially done therefore NACK
LS
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 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 >> >> 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
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 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 >>> >>> 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.
On (17/12/14 15:27), Pavel Reichl wrote:
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 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 >>>> >>>>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
Updated patches attached.
ACK
LS
On Wed, Dec 17, 2014 at 03:47:05PM +0100, Lukas Slebodnik wrote:
On (17/12/14 15:27), Pavel Reichl wrote:
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 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 >>>>> >>>>>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
Updated patches attached.
ACK
Pushed to master: 13c0cf829eca7891ad9d0087e91c72650f990149 d7b90921c1a404f0d9fb8384a8fd55fd15b86916 4bbcc2d6d3f16b015796818746a45134861c93a4
bye, Sumit
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org
-
Lukas Slebodnik -
Michal Židek -
Pavel Reichl -
Sumit Bose