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(a)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(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel