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.
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