Hi,
the attached patches fix the crash as described in https://fedorahosted.org/sssd/ticket/2598 and add a unit test. I hope the patch descriptions are fine.
Thanks for review.
On Mon, Mar 09, 2015 at 06:17:50PM +0100, Jakub Hrozek wrote:
Hi,
the attached patches fix the crash as described in https://fedorahosted.org/sssd/ticket/2598 and add a unit test. I hope the patch descriptions are fine.
Thanks for review.
attached are patches rebased on top of my cmocka test changes.
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:
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:
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.
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:
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@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
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:
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@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
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
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:
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@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:
You did not test each patch separatelly. You tested only a whole patchset.
LS
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:
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@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:
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@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Mar 11, 2015 at 06:02:01PM +0100, 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:
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@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:
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.
On 03/11/2015 06:07 PM, Jakub Hrozek wrote:
On Wed, Mar 11, 2015 at 06:02:01PM +0100, Pavel Reichl wrote:
<blockquote>
On 03/11/2015 05:04 PM, Lukas Slebodnik wrote:
<blockquote>
On (11/03/15 16:48), Jakub Hrozek wrote:
<blockquote>
On Wed, Mar 11, 2015 at 04:30:52PM +0100, Pavel Reichl wrote:
<blockquote>
On 03/11/2015 03:46 PM, Jakub Hrozek wrote:
<blockquote>
@@ -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
</blockquote>
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.
</blockquote>
</blockquote>
I think this version looks a little better, but I'm fine with both version. So ACK from me.
<blockquote>
From c0daf41e1c9ca1e8331dc9e5e7098e7862fd8b7e Mon Sep 17 00:00:00 2001
<blockquote>
From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 9 Mar 2015 16:59:39 +0100 Subject: [PATCH 3/4] Add unit tests for initgroups
</blockquote>
This patch should be applied as the last one (and not 3rd) otherwise the test nss-srv-tests fails and our CI would complain.
</blockquote>
How would CI complain? It seems OK to me: http://sssd-ci.duckdns.org/logs/job/9/47/summary.html
</blockquote>
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.
</blockquote>
I quickly retested the patchset. ACK ci passed: http://sssd-ci.duckdns.org/logs/job/9/49/summary.html
<blockquote>
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
</blockquote>
On Wed, Mar 11, 2015 at 06:35:16PM -0400, Pavel Reichl wrote:
On 03/11/2015 06:07 PM, Jakub Hrozek wrote:
On Wed, Mar 11, 2015 at 06:02:01PM +0100, Pavel Reichl wrote:
<blockquote>
On 03/11/2015 05:04 PM, Lukas Slebodnik wrote:
<blockquote>
On (11/03/15 16:48), Jakub Hrozek wrote:
<blockquote>
On Wed, Mar 11, 2015 at 04:30:52PM +0100, Pavel Reichl wrote:
<blockquote>
On 03/11/2015 03:46 PM, Jakub Hrozek wrote:
<blockquote>
@@ -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
</blockquote>
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.
</blockquote>
</blockquote>
I think this version looks a little better, but I'm fine with both version. So ACK from me.
<blockquote>
From c0daf41e1c9ca1e8331dc9e5e7098e7862fd8b7e Mon Sep 17 00:00:00 2001
<blockquote>
From: Jakub Hrozek jhrozek@redhat.com Date: Mon, 9 Mar 2015 16:59:39 +0100 Subject: [PATCH 3/4] Add unit tests for initgroups
</blockquote>
This patch should be applied as the last one (and not 3rd) otherwise the test nss-srv-tests fails and our CI would complain.
</blockquote>
How would CI complain? It seems OK to me: http://sssd-ci.duckdns.org/logs/job/9/47/summary.html
</blockquote>
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.
</blockquote>
I quickly retested the patchset. ACK ci passed: http://sssd-ci.duckdns.org/logs/job/9/49/summary.html
Thanks, pushed upstream:
* master: * aa648535f445e7a95bf6dedc7c43bb5f94ab7354 * ff19b24a93a50c8a62b5c2621e45d101e3a00781 * 84a4c4fcc93b3dcc70604817a05f7943606ff596 * 9cc2223e0bc0478c1b47a47fd71bba7e7129492d * sssd-1-12: * d8006abd55f2ce0698e09213b8374e9071e70016
I only pushed the fix without the unit tests to upstream, since the unit tests were already converted to cmocka-1.0
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Pavel Reichl