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