[SSSD] [PATCH] NSS: Handle ENOENT when doing initgroups by UPN
Pavel Reichl
preichl at redhat.com
Wed Mar 11 17:10:33 UTC 2015
On 03/11/2015 06:04 PM, Lukas Slebodnik wrote:
> On (11/03/15 18:02), Pavel Reichl wrote:
>>
>> On 03/11/2015 05:04 PM, Lukas Slebodnik wrote:
>>> On (11/03/15 16:48), Jakub Hrozek wrote:
>>>> On Wed, Mar 11, 2015 at 04:30:52PM +0100, Pavel Reichl wrote:
>>>>> On 03/11/2015 03:46 PM, Jakub Hrozek wrote:
>>>>>
>>>>>
>>>>>> @@ -4062,29 +4062,39 @@ static int nss_cmd_initgroups_search(struct nss_dom_ctx *dctx)
>>>>>> if (cmdctx->name_is_upn) {
>>>>>> ret = sysdb_search_user_by_upn(cmdctx, dom, name, user_attrs, &msg);
>>>>>> - if (ret != EOK && ret != ENOENT) {
>>>>>> + if (ret == EOK) {
>>>>>> + sysdb_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
>>>>>> + if (sysdb_name == NULL) {
>>>>>> + DEBUG(SSSDBG_OP_FAILURE,
>>>>>> + "Sysdb entry does not have a name.\n");
>>>>>> + return EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + ret = sysdb_initgroups(cmdctx, dom, sysdb_name, &dctx->res);
>>>>>> + if (ret == EOK && DOM_HAS_VIEWS(dom)) {
>>>>>> + for (c = 0; c < dctx->res->count; c++) {
>>>>>> + ret = sysdb_add_overrides_to_object(dom, dctx->res->msgs[c],
>>>>>> + NULL, NULL);
>>>>>> + if (ret != EOK) {
>>>>>> + DEBUG(SSSDBG_OP_FAILURE,
>>>>>> + "sysdb_add_overrides_to_object failed.\n");
>>>>>> + return ret;
>>>>>> + }
>>>>>> + }
>>>>>> + }
>>>>>> + } else if (ret != ENOENT) {
>>>>>> DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn failed.\n");
>>>>>> return ret;
>>>>>> - }
>>>>>> -
>>>>>> - sysdb_name = ldb_msg_find_attr_as_string(msg, SYSDB_NAME, NULL);
>>>>>> - if (sysdb_name == NULL) {
>>>>>> - DEBUG(SSSDBG_OP_FAILURE,
>>>>>> - "Sysdb entry does not have a name.\n");
>>>>>> - return EINVAL;
>>>>>> - }
>>>>>> -
>>>>>> - ret = sysdb_initgroups(cmdctx, dom, sysdb_name, &dctx->res);
>>>>>> - if (ret == EOK && DOM_HAS_VIEWS(dom)) {
>>>>>> - for (c = 0; c < dctx->res->count; c++) {
>>>>>> - ret = sysdb_add_overrides_to_object(dom, dctx->res->msgs[c],
>>>>>> - NULL, NULL);
>>>>>> - if (ret != EOK) {
>>>>>> - DEBUG(SSSDBG_OP_FAILURE,
>>>>>> - "sysdb_add_overrides_to_object failed.\n");
>>>>>> - return ret;
>>>>>> - }
>>>>>> + } else {
>>>>>> + dctx->res = talloc_zero(cmdctx, struct ldb_result);
>>>>>> + if (dctx->res == NULL) {
>>>>>> + DEBUG(SSSDBG_OP_FAILURE, "talloc_zero failed.\n");
>>>>>> + return ENOMEM;
>>>>>> }
>>>>>> +
>>>>>> + dctx->res->count = 0;
>>>>>> + dctx->res->msgs = NULL;
>>>>>> + ret = EOK;
>>>>>> }
>>>>>> } else {
>>>>>> ret = sysdb_initgroups_with_views(cmdctx, dom, name, &dctx->res);
>>>>>> -- 2.1.0
>>>>> Thanks for the patches.
>>>>> All patches LGTM.
>>>>>
>>>>> I've tested that tests fail if the forth patch is not applied. I also wrote
>>>>> simple program to reproduce the segfault and after applying the 4th patch
>>>>> I'm no longer able to reproduce the bug.
>>>>>
>>>>> I have only one style nitpick - would it make more sense to test explicitly
>>>>> for 'ret == ENOENT' and handle the other cases in else branch?
>>>>>
>>>>> If you don't agree then ignore then comment and push the patches (ACK to
>>>>> all).
>>>>>
>>>>> Thanks!
>>>>>
>>>>> ci passed:
>>>>>
>>>>> http://sssd-ci.duckdns.org/logs/job/9/46/summary.html
>>>> I'm not sure if I understood what you meant, but check the attached
>>>> patches.
>>>>
>>>> Normally I prefer:
>>>> if (ret == ENOENT) {
>>>> /* handle ENOENT */
>>>> } else if (ret != EOK) {
>>>> /* Error! *//
>>>> }
>>>>
>>>> /* Let's not put EOK into else and avoid extra indentation */
>>>>
>>>> But here the EOK must be in an else branch, I think.
>> I think this version looks a little better, but I'm fine with both version.
>> So ACK from me.
>>
>>
>>> >From c0daf41e1c9ca1e8331dc9e5e7098e7862fd8b7e Mon Sep 17 00:00:00 2001
>>>> From: Jakub Hrozek <jhrozek at redhat.com>
>>>> Date: Mon, 9 Mar 2015 16:59:39 +0100
>>>> Subject: [PATCH 3/4] Add unit tests for initgroups
>>>>
>>> This patch should be applied as the last one (and not 3rd)
>>> otherwise the test nss-srv-tests fails and our CI would complain.
>> How would CI complain? It seems OK to me:
>>
>> http://sssd-ci.duckdns.org/logs/job/9/47/summary.html
>>
> You did not test each patch separatelly.
> You tested only a whole patchset.
Oh, Lukas just remained me, that the test failure would be a problem for
git bisect. Then I guess we should move the 4th patch.
>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list