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
I think that's because the CI that runs post-commit runs the 'rigorous'
set of tests which applies the commits one by one. Developers usually
run the 'essential' set of tests.
The patch order was actually intentional, I was first trying to show the
bug and fix in the later patch. But I didn't realize the CI point. I
think Lukas is right.
Attached is the patchset I'd like to push.