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