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