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