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 <prei...@redhat.com> >>>>>>>Date: Tue, 9 Dec 2014 09:47:11 +0000 >>>>>>>Subject: [PATCH 1/2] TESTS: sysdb_delete_by_sid() test return value >>>>>>> >>>>>>>Check that return value of sysdb_delete_by_sid() is not changed as >>>>>>>called SYSDB functions have changed the return value. >>>>>>> >>>>>>>Part of patches for: >>>>>>>https://fedorahosted.org/sssd/ticket/1991 >>>>>>>--- >>>>>>LGTM >>>>>> >>>>>>>From 1cb395244d3ab3906af5b6c8b294e80055ad0a07 Mon Sep 17 00:00:00 2001 >>>>>>>From: Pavel Reichl <prei...@redhat.com> >>>>>>>Date: Tue, 9 Dec 2014 11:01:13 +0000 >>>>>>>Subject: [PATCH 2/2] SYSDB: sysdb_search_object_by_sid returns ENOENT >>>>>>> >>>>>>>sysdb_search_object_by_sid returns ENOENT if no results are found. >>>>>>> >>>>>>>Part od solution for: >>>>>>>https://fedorahosted.org/sssd/ticket/1991 >>>>>>>--- >>>>>>>src/db/sysdb.h | 2 +- >>>>>>>src/db/sysdb_ops.c | 70 >>>>>>>++++++------------------------------------ >>>>>>>src/responder/nss/nsssrv_cmd.c | 27 ++++++++-------- >>>>>>>src/responder/pac/pacsrv_cmd.c | 26 ++++++++++------ >>>>>>>src/tests/sysdb-tests.c | 5 +-- >>>>>>>5 files changed, 42 insertions(+), 88 deletions(-) >>>>>>> >>>>>>>diff --git a/src/db/sysdb.h b/src/db/sysdb.h >>>>>>>index >>>>>>>5bd7f90acb685bbaff5c98f433c7dce8175c33ca..9b88b4a63619456f8f3cc1961e29bbf3946ca5b8 >>>>>>> 100644 >>>>>>>--- a/src/db/sysdb.h >>>>>>>+++ b/src/db/sysdb.h >>>>>>>@@ -1033,7 +1033,7 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX >>>>>>>*mem_ctx, >>>>>>> struct sss_domain_info *domain, >>>>>>> const char *sid_str, >>>>>>> const char **attrs, >>>>>>>- struct ldb_result **msg); >>>>>>>+ struct ldb_result **res); >>>>>>> >>>>>>>errno_t sysdb_search_object_by_uuid(TALLOC_CTX *mem_ctx, >>>>>>> struct sss_domain_info *domain, >>>>>>>diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c >>>>>>>index >>>>>>>998046a2ca1c746b2032f430e5f9c4a7151e1dbc..8d6b11c248fd7f895ff6a95c25d4372fc7f8445d >>>>>>> 100644 >>>>>>>--- a/src/db/sysdb_ops.c >>>>>>>+++ b/src/db/sysdb_ops.c >>>>>>>@@ -2995,8 +2995,15 @@ int sysdb_delete_by_sid(struct sysdb_ctx *sysdb, >>>>>>> >>>>>>> ret = sysdb_search_object_by_sid(tmp_ctx, domain, sid_str, NULL, >>>>>>> &res); >>>>>>> if (ret != EOK) { >>>>>>>- DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: %d (%s)\n", >>>>>>>- ret, strerror(ret)); >>>>>>>+ if (ret == ENOENT) { >>>>>>>+ /* No existing entry. Just quit. */ >>>>>>>+ DEBUG(SSSDBG_TRACE_FUNC, >>>>>>>+ "search by sid did not return any results.\n"); >>>>>>>+ ret = EOK; >>>>>>>+ } else { >>>>>>>+ DEBUG(SSSDBG_OP_FAILURE, "search by sid failed: %d (%s)\n", >>>>>>>+ ret, strerror(ret)); >>>>>>>+ } >>>>>>> goto done; >>>>>>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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
