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