Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
Thanks,
Pavel Reichl
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
The first patch looks good, I'll run some tests and will give my results later.
bye, Sumit
Thanks,
Pavel Reichl
On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
The first patch looks good, I'll run some tests and will give my results later.
ACK to the first patch.
bye, Sumit
bye, Sumit
Thanks,
Pavel Reichl
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (08/04/14 13:00), Sumit Bose wrote:
On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
The first patch looks good, I'll run some tests and will give my results later.
ACK to the first patch.
bye, Sumit
Not all return point from function pac_lookup_sids_done were converted using goto.
-- 305- } else if (msg->count > 1) { 306- DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \ 307- "for SID [%s].\n", 308- entries[c].key.str); 309- talloc_free(msg); 310: pac_cmd_done(cctx, EINVAL); 311- return; ^^^^^^^ here 312- } 313- 314- id = ldb_msg_find_attr_as_uint64(msg->msgs[0], 315- SYSDB_UIDNUM, 0); --
LS
On Tue, 2014-04-08 at 13:23 +0200, Lukas Slebodnik wrote:
On (08/04/14 13:00), Sumit Bose wrote:
On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
The first patch looks good, I'll run some tests and will give my results later.
ACK to the first patch.
bye, Sumit
Not all return point from function pac_lookup_sids_done were converted using goto.
-- 305- } else if (msg->count > 1) { 306- DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \ 307- "for SID [%s].\n", 308- entries[c].key.str); 309- talloc_free(msg); 310: pac_cmd_done(cctx, EINVAL); 311- return; ^^^^^^^ here 312- } 313- 314- id = ldb_msg_find_attr_as_uint64(msg->msgs[0], 315- SYSDB_UIDNUM, 0); --
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thank you Lukas for your input. You complicated the situation one more time. :-)
I had more careful look on the code and I found several places that seems troublesome to me:
static void pac_lookup_sids_done(struct tevent_req *req) { struct pac_req_ctx *pr_ctx = tevent_req_callback_data(req, struct pac_req_ctx); struct cli_ctx *cctx = pr_ctx->cctx;
[snip]
ret = pac_lookup_sids_recv(req); talloc_zfree(req);
[snip] ret = hash_entries(pr_ctx->sid_table, &count, &entries);
Is this use of pr_ctx->sid use-after-free bug as it was allocated in req which was already freed?
talloc_free(pr_ctx); pac_cmd_done(cctx, ret);
cctx points to pr_ctx->cctx, but pr_ctx is freed before call of pac_cmd_done(cctx, ret) - use-after-free?
I don't feel confident enough to make any changes here without approval of somebody more acquainted with this code.
On Wed, Apr 09, 2014 at 05:22:16PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-08 at 13:23 +0200, Lukas Slebodnik wrote:
On (08/04/14 13:00), Sumit Bose wrote:
On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
The first patch looks good, I'll run some tests and will give my results later.
ACK to the first patch.
bye, Sumit
Not all return point from function pac_lookup_sids_done were converted using goto.
-- 305- } else if (msg->count > 1) { 306- DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \ 307- "for SID [%s].\n", 308- entries[c].key.str); 309- talloc_free(msg); 310: pac_cmd_done(cctx, EINVAL); 311- return; ^^^^^^^ here 312- } 313- 314- id = ldb_msg_find_attr_as_uint64(msg->msgs[0], 315- SYSDB_UIDNUM, 0); --
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thank you Lukas for your input. You complicated the situation one more time. :-)
I had more careful look on the code and I found several places that seems troublesome to me:
no, if you add a bit of more code:
static errno_t pac_resolve_sids_next(struct pac_req_ctx *pr_ctx) [snip] req = pac_lookup_sids_send(pr_ctx, pr_ctx->cctx->ev, pr_ctx, pr_ctx->pac_ctx, pr_ctx->sid_table); if (req == NULL) { DEBUG(SSSDBG_OP_FAILURE, "pac_lookup_sids_send failed.\n"); return ENOMEM; }
tevent_req_set_callback(req, pac_lookup_sids_done, pr_ctx);
you can see that req is allocated on pr_ctx, i.e. req is a child of pr_ctx. Freeing req does not free pr_ctx (but freeing pr_ctx would free req as well). The hierarchy in the struct is a different one than in the talloc tree.
static void pac_lookup_sids_done(struct tevent_req *req) { struct pac_req_ctx *pr_ctx = tevent_req_callback_data(req, struct pac_req_ctx); struct cli_ctx *cctx = pr_ctx->cctx;
[snip]
ret = pac_lookup_sids_recv(req); talloc_zfree(req);
[snip] ret = hash_entries(pr_ctx->sid_table, &count, &entries);
Is this use of pr_ctx->sid use-after-free bug as it was allocated in req which was already freed?
talloc_free(pr_ctx); pac_cmd_done(cctx, ret);
cctx points to pr_ctx->cctx, but pr_ctx is freed before call of pac_cmd_done(cctx, ret) - use-after-free?
same here:
static errno_t pac_add_pac_user(struct cli_ctx *cctx) [snip] pr_ctx = talloc_zero(cctx, struct pac_req_ctx);
pt_ctx is a child of cctx talloc-wise.
HTH
bye, Sumit
I don't feel confident enough to make any changes here without approval of somebody more acquainted with this code.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, 2014-04-09 at 21:16 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 05:22:16PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-08 at 13:23 +0200, Lukas Slebodnik wrote:
On (08/04/14 13:00), Sumit Bose wrote:
On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
The first patch looks good, I'll run some tests and will give my results later.
ACK to the first patch.
bye, Sumit
Not all return point from function pac_lookup_sids_done were converted using goto.
-- 305- } else if (msg->count > 1) { 306- DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \ 307- "for SID [%s].\n", 308- entries[c].key.str); 309- talloc_free(msg); 310: pac_cmd_done(cctx, EINVAL); 311- return; ^^^^^^^ here 312- } 313- 314- id = ldb_msg_find_attr_as_uint64(msg->msgs[0], 315- SYSDB_UIDNUM, 0); --
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thank you Lukas for your input. You complicated the situation one more time. :-)
I had more careful look on the code and I found several places that seems troublesome to me:
no, if you add a bit of more code:
static errno_t pac_resolve_sids_next(struct pac_req_ctx *pr_ctx) [snip] req = pac_lookup_sids_send(pr_ctx, pr_ctx->cctx->ev, pr_ctx, pr_ctx->pac_ctx, pr_ctx->sid_table); if (req == NULL) { DEBUG(SSSDBG_OP_FAILURE, "pac_lookup_sids_send failed.\n"); return ENOMEM; }
tevent_req_set_callback(req, pac_lookup_sids_done, pr_ctx);
you can see that req is allocated on pr_ctx, i.e. req is a child of pr_ctx. Freeing req does not free pr_ctx (but freeing pr_ctx would free req as well). The hierarchy in the struct is a different one than in the talloc tree.
static void pac_lookup_sids_done(struct tevent_req *req) { struct pac_req_ctx *pr_ctx = tevent_req_callback_data(req, struct pac_req_ctx); struct cli_ctx *cctx = pr_ctx->cctx;
[snip]
ret = pac_lookup_sids_recv(req); talloc_zfree(req);
[snip] ret = hash_entries(pr_ctx->sid_table, &count, &entries);
Is this use of pr_ctx->sid use-after-free bug as it was allocated in req which was already freed?
talloc_free(pr_ctx); pac_cmd_done(cctx, ret);
cctx points to pr_ctx->cctx, but pr_ctx is freed before call of pac_cmd_done(cctx, ret) - use-after-free?
same here:
static errno_t pac_add_pac_user(struct cli_ctx *cctx) [snip] pr_ctx = talloc_zero(cctx, struct pac_req_ctx);
pt_ctx is a child of cctx talloc-wise.
HTH
bye, Sumit
I don't feel confident enough to make any changes here without approval of somebody more acquainted with this code.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello Sumit, thank you for explaining it to me - lesson learned here.
Lukas, would attached patch work better for you?
On Thu, 2014-04-10 at 10:37 +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:16 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 05:22:16PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-08 at 13:23 +0200, Lukas Slebodnik wrote:
On (08/04/14 13:00), Sumit Bose wrote:
On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > Hello, > > I noticed these two warnings in clang. > > It would be great if the 2nd patch could be checked by Sumit to make > sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
The first patch looks good, I'll run some tests and will give my results later.
ACK to the first patch.
bye, Sumit
Not all return point from function pac_lookup_sids_done were converted using goto.
-- 305- } else if (msg->count > 1) { 306- DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \ 307- "for SID [%s].\n", 308- entries[c].key.str); 309- talloc_free(msg); 310: pac_cmd_done(cctx, EINVAL); 311- return; ^^^^^^^ here 312- } 313- 314- id = ldb_msg_find_attr_as_uint64(msg->msgs[0], 315- SYSDB_UIDNUM, 0); --
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thank you Lukas for your input. You complicated the situation one more time. :-)
I had more careful look on the code and I found several places that seems troublesome to me:
no, if you add a bit of more code:
static errno_t pac_resolve_sids_next(struct pac_req_ctx *pr_ctx) [snip] req = pac_lookup_sids_send(pr_ctx, pr_ctx->cctx->ev, pr_ctx, pr_ctx->pac_ctx, pr_ctx->sid_table); if (req == NULL) { DEBUG(SSSDBG_OP_FAILURE, "pac_lookup_sids_send failed.\n"); return ENOMEM; }
tevent_req_set_callback(req, pac_lookup_sids_done, pr_ctx);
you can see that req is allocated on pr_ctx, i.e. req is a child of pr_ctx. Freeing req does not free pr_ctx (but freeing pr_ctx would free req as well). The hierarchy in the struct is a different one than in the talloc tree.
static void pac_lookup_sids_done(struct tevent_req *req) { struct pac_req_ctx *pr_ctx = tevent_req_callback_data(req, struct pac_req_ctx); struct cli_ctx *cctx = pr_ctx->cctx;
[snip]
ret = pac_lookup_sids_recv(req); talloc_zfree(req);
[snip] ret = hash_entries(pr_ctx->sid_table, &count, &entries);
Is this use of pr_ctx->sid use-after-free bug as it was allocated in req which was already freed?
talloc_free(pr_ctx); pac_cmd_done(cctx, ret);
cctx points to pr_ctx->cctx, but pr_ctx is freed before call of pac_cmd_done(cctx, ret) - use-after-free?
same here:
static errno_t pac_add_pac_user(struct cli_ctx *cctx) [snip] pr_ctx = talloc_zero(cctx, struct pac_req_ctx);
pt_ctx is a child of cctx talloc-wise.
HTH
bye, Sumit
I don't feel confident enough to make any changes here without approval of somebody more acquainted with this code.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello Sumit, thank you for explaining it to me - lesson learned here.
Lukas, would attached patch work better for you?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello,
Lukas, do you like this version of the patch or shall we go with previous version of patch, which is already ACKed by Sumit?
On (15/04/14 16:41), Pavel Reichl wrote:
On Thu, 2014-04-10 at 10:37 +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:16 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 05:22:16PM +0200, Pavel Reichl wrote:
On Tue, 2014-04-08 at 13:23 +0200, Lukas Slebodnik wrote:
On (08/04/14 13:00), Sumit Bose wrote:
On Mon, Apr 07, 2014 at 06:13:53PM +0200, Sumit Bose wrote: > On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > > Hello, > > > > I noticed these two warnings in clang. > > > > It would be great if the 2nd patch could be checked by Sumit to make > > sure that the return value wasn't ignored on purpose. > > yes, I would prefer to ignore errors here. There might be various cases > were we are not able to resolve a single SID but still can proceed with > the others. > > The first patch looks good, I'll run some tests and will give my results > later.
ACK to the first patch.
bye, Sumit
Not all return point from function pac_lookup_sids_done were converted using goto.
-- 305- } else if (msg->count > 1) { 306- DEBUG(SSSDBG_CRIT_FAILURE, "More then one result returned " \ 307- "for SID [%s].\n", 308- entries[c].key.str); 309- talloc_free(msg); 310: pac_cmd_done(cctx, EINVAL); 311- return; ^^^^^^^ here 312- } 313- 314- id = ldb_msg_find_attr_as_uint64(msg->msgs[0], 315- SYSDB_UIDNUM, 0); --
LS
Thank you Lukas for your input. You complicated the situation one more time. :-)
I had more careful look on the code and I found several places that seems troublesome to me:
no, if you add a bit of more code:
static errno_t pac_resolve_sids_next(struct pac_req_ctx *pr_ctx) [snip] req = pac_lookup_sids_send(pr_ctx, pr_ctx->cctx->ev, pr_ctx, pr_ctx->pac_ctx, pr_ctx->sid_table); if (req == NULL) { DEBUG(SSSDBG_OP_FAILURE, "pac_lookup_sids_send failed.\n"); return ENOMEM; }
tevent_req_set_callback(req, pac_lookup_sids_done, pr_ctx);
you can see that req is allocated on pr_ctx, i.e. req is a child of pr_ctx. Freeing req does not free pr_ctx (but freeing pr_ctx would free req as well). The hierarchy in the struct is a different one than in the talloc tree.
static void pac_lookup_sids_done(struct tevent_req *req) { struct pac_req_ctx *pr_ctx = tevent_req_callback_data(req, struct pac_req_ctx); struct cli_ctx *cctx = pr_ctx->cctx;
[snip]
ret = pac_lookup_sids_recv(req); talloc_zfree(req);
[snip] ret = hash_entries(pr_ctx->sid_table, &count, &entries);
Is this use of pr_ctx->sid use-after-free bug as it was allocated in req which was already freed?
talloc_free(pr_ctx); pac_cmd_done(cctx, ret);
cctx points to pr_ctx->cctx, but pr_ctx is freed before call of pac_cmd_done(cctx, ret) - use-after-free?
same here:
static errno_t pac_add_pac_user(struct cli_ctx *cctx) [snip] pr_ctx = talloc_zero(cctx, struct pac_req_ctx);
pt_ctx is a child of cctx talloc-wise.
HTH
bye, Sumit
I don't feel confident enough to make any changes here without approval of somebody more acquainted with this code.
Hello Sumit, thank you for explaining it to me - lesson learned here.
Lukas, would attached patch work better for you?
Hello,
Lukas, do you like this version of the patch or shall we go with previous version of patch, which is already ACKed by Sumit?
Your patches do not fix any real problem. RFEs for 1.12 (or 1.11) have higher priority. e.g. https://fedorahosted.org/sssd/ticket/1853
I would need to test patches with static analysers; I can review patches on Thursday or later.
LS
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
The first patch looks good, I'll run some tests and will give my results later.
bye, Sumit
Thanks,
Pavel Reichl
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
bye, Sumit
The first patch looks good, I'll run some tests and will give my results later.
bye, Sumit
Thanks,
Pavel Reichl
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
From 9f4cb8181f9d7aae4f66998fc79e776ca1c58e90 Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 9 Apr 2014 18:00:56 +0100 Subject: [PATCH] PAC: fix clang warning
Return value of sss_dp_get_account_recv is ignored on purpose. To avoid clang warning cast to void is used.
src/responder/pac/pacsrv_cmd.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c index 2fe76d1cf5e095bd89513ad5d72608dabea0939f..0ccd98c00cf798bd9a33cd27374fd3cf2d3fb435 100644 --- a/src/responder/pac/pacsrv_cmd.c +++ b/src/responder/pac/pacsrv_cmd.c @@ -1092,6 +1092,9 @@ static void pac_lookup_sids_next_done(struct tevent_req *subreq) ret = sss_dp_get_account_recv(req, subreq, &err_maj, &err_min, &err_msg);
- /* To avoid clang warning about 'dead assignment'. */
- (void) ret;
- talloc_zfree(subreq); talloc_zfree(err_msg); /* Errors during individual lookups are ignored. */
-- 1.8.4.2
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (09/04/14 21:19), Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
+1
LS
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Pavel Reichl
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
bye, Sumit
Pavel Reichl
From eb7bff126d870b2484bcfccf1983a00783985c0b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 9 Apr 2014 18:00:56 +0100 Subject: [PATCH 2/2] PAC: fix clang warning
Return value of sss_dp_get_account_recv is checked and logging is done accordingly.
src/responder/pac/pacsrv_cmd.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c index 00cdf608bef2be1543a15cee4ae02c78c3bc9f8e..cf631ff360728a1be21f7cec4128b4d57e0d3a08 100644 --- a/src/responder/pac/pacsrv_cmd.c +++ b/src/responder/pac/pacsrv_cmd.c @@ -1094,6 +1094,14 @@ static void pac_lookup_sids_next_done(struct tevent_req *subreq) ret = sss_dp_get_account_recv(req, subreq, &err_maj, &err_min, &err_msg);
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
"Unable to get information from Data Provider\n"
"dp_error: [%u], errno: [%u], error_msg: [%s]\n",
(unsigned int)err_maj, (unsigned int)err_min,
err_msg ? err_msg : "none");
- }
- talloc_zfree(subreq); talloc_zfree(err_msg); /* Errors during individual lookups are ignored. */
-- 1.8.4.2
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Apr 10, 2014 at 11:11:51AM +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
Pushed to master: 3c11d2418677e889f2eb38e30480e008b04c4140
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote:
Hello,
I noticed these two warnings in clang.
It would be great if the 2nd patch could be checked by Sumit to make sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Pavel Reichl
bye, Sumit
Pavel Reichl
From eb7bff126d870b2484bcfccf1983a00783985c0b Mon Sep 17 00:00:00 2001 From: Pavel Reichl preichl@redhat.com Date: Wed, 9 Apr 2014 18:00:56 +0100 Subject: [PATCH 2/2] PAC: fix clang warning
Return value of sss_dp_get_account_recv is checked and logging is done accordingly.
src/responder/pac/pacsrv_cmd.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c index 00cdf608bef2be1543a15cee4ae02c78c3bc9f8e..cf631ff360728a1be21f7cec4128b4d57e0d3a08 100644 --- a/src/responder/pac/pacsrv_cmd.c +++ b/src/responder/pac/pacsrv_cmd.c @@ -1094,6 +1094,14 @@ static void pac_lookup_sids_next_done(struct tevent_req *subreq) ret = sss_dp_get_account_recv(req, subreq, &err_maj, &err_min, &err_msg);
- if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE,
"Unable to get information from Data Provider\n"
"dp_error: [%u], errno: [%u], error_msg: [%s]\n",
(unsigned int)err_maj, (unsigned int)err_min,
err_msg ? err_msg : "none");
- }
- talloc_zfree(subreq); talloc_zfree(err_msg); /* Errors during individual lookups are ignored. */
-- 1.8.4.2
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Thu, Apr 10, 2014 at 12:11:05PM +0200, Pavel Reichl wrote:
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > Hello, > > I noticed these two warnings in clang. > > It would be great if the 2nd patch could be checked by Sumit to make > sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Pavel Reichl
Yes, I think the convention dates back from when we didn't have the fixed-size conversion macros so we used to cast the variable to a wider type. I don't think it's required to change existing code, though, but should start using the precise conversions in new code.
On (10/04/14 12:11), Pavel Reichl wrote:
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote:
On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > Hello, > > I noticed these two warnings in clang. > > It would be great if the 2nd patch could be checked by Sumit to make > sure that the return value wasn't ignored on purpose.
yes, I would prefer to ignore errors here. There might be various cases were we are not able to resolve a single SID but still can proceed with the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any
If you copy&past this code then explicit casting is in two places. In future, it can in 3, 4, 5 ...
relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Is it a problem to send another patch?
LS
On Thu, Apr 10, 2014 at 01:44:08PM +0200, Lukas Slebodnik wrote:
On (10/04/14 12:11), Pavel Reichl wrote:
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote:
On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote: > On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > > Hello, > > > > I noticed these two warnings in clang. > > > > It would be great if the 2nd patch could be checked by Sumit to make > > sure that the return value wasn't ignored on purpose. > > yes, I would prefer to ignore errors here. There might be various cases > were we are not able to resolve a single SID but still can proceed with > the others.
Please see attached patch. Feel free to NACK it, if you think it's more pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any
If you copy&past this code then explicit casting is in two places. In future, it can in 3, 4, 5 ...
relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Is it a problem to send another patch?
LS
If you have time to convert the debug messages, then by all means, send a patch. Maybe this could be a nice intern task? :-)
On (10/04/14 14:10), Jakub Hrozek wrote:
On Thu, Apr 10, 2014 at 01:44:08PM +0200, Lukas Slebodnik wrote:
On (10/04/14 12:11), Pavel Reichl wrote:
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote:
On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote: > On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote: > > On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > > > Hello, > > > > > > I noticed these two warnings in clang. > > > > > > It would be great if the 2nd patch could be checked by Sumit to make > > > sure that the return value wasn't ignored on purpose. > > > > yes, I would prefer to ignore errors here. There might be various cases > > were we are not able to resolve a single SID but still can proceed with > > the others. > > Please see attached patch. Feel free to NACK it, if you think it's more > pain than gain.
Why not do something useful to avoid the compiler warning and print a SSSDBG_TRACE_ALL debug message with the returned error code in the case of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any
If you copy&past this code then explicit casting is in two places. In future, it can in 3, 4, 5 ...
relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Is it a problem to send another patch?
LS
If you have time to convert the debug messages, then by all means, send a patch. Maybe this could be a nice intern task? :-)
I think it should be the same case as with "converting old debug levels to new one" If you see casting you will fix it. (maybe in separate patch)
LS
On (10/04/14 15:05), Lukas Slebodnik wrote:
On (10/04/14 14:10), Jakub Hrozek wrote:
On Thu, Apr 10, 2014 at 01:44:08PM +0200, Lukas Slebodnik wrote:
On (10/04/14 12:11), Pavel Reichl wrote:
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote:
On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote: > On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote: > > On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote: > > > On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > > > > Hello, > > > > > > > > I noticed these two warnings in clang. > > > > > > > > It would be great if the 2nd patch could be checked by Sumit to make > > > > sure that the return value wasn't ignored on purpose. > > > > > > yes, I would prefer to ignore errors here. There might be various cases > > > were we are not able to resolve a single SID but still can proceed with > > > the others. > > > > Please see attached patch. Feel free to NACK it, if you think it's more > > pain than gain. > > Why not do something useful to avoid the compiler warning and print a > SSSDBG_TRACE_ALL debug message with the returned error code in the case > of an error?
Hello Sumit,
I just took your previous response too literally - "I would prefer to ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any
If you copy&past this code then explicit casting is in two places. In future, it can in 3, 4, 5 ...
relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Is it a problem to send another patch?
LS
If you have time to convert the debug messages, then by all means, send a patch. Maybe this could be a nice intern task? :-)
What about this patch?
LS
On (26/08/14 13:41), Lukas Slebodnik wrote:
On (10/04/14 15:05), Lukas Slebodnik wrote:
On (10/04/14 14:10), Jakub Hrozek wrote:
On Thu, Apr 10, 2014 at 01:44:08PM +0200, Lukas Slebodnik wrote:
On (10/04/14 12:11), Pavel Reichl wrote:
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote: > On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote: > > On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote: > > > On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote: > > > > On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: > > > > > Hello, > > > > > > > > > > I noticed these two warnings in clang. > > > > > > > > > > It would be great if the 2nd patch could be checked by Sumit to make > > > > > sure that the return value wasn't ignored on purpose. > > > > > > > > yes, I would prefer to ignore errors here. There might be various cases > > > > were we are not able to resolve a single SID but still can proceed with > > > > the others. > > > > > > Please see attached patch. Feel free to NACK it, if you think it's more > > > pain than gain. > > > > Why not do something useful to avoid the compiler warning and print a > > SSSDBG_TRACE_ALL debug message with the returned error code in the case > > of an error? > > Hello Sumit, > > I just took your previous response too literally - "I would prefer to > ignore errors here" - my bad. (Hopefully final) patch attached.
Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any
If you copy&past this code then explicit casting is in two places. In future, it can in 3, 4, 5 ...
relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Is it a problem to send another patch?
LS
If you have time to convert the debug messages, then by all means, send a patch. Maybe this could be a nice intern task? :-)
What about this patch?
LS
From b7d33f8ef584cb1e57fb4a286959a9ddf8aed7f8 Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 26 Aug 2014 11:35:23 +0200 Subject: [PATCH 1/2] SDAP: Fix using of uninitialized variable
When group was posix and id mapping was not enabled then variable gid was used uninitialized.
Fixing commit description: id mapping *must be* enabled.
updated version attached.
LS
On 08/26/2014 01:41 PM, Lukas Slebodnik wrote:
On (10/04/14 15:05), Lukas Slebodnik wrote:
On (10/04/14 14:10), Jakub Hrozek wrote:
On Thu, Apr 10, 2014 at 01:44:08PM +0200, Lukas Slebodnik wrote:
On (10/04/14 12:11), Pavel Reichl wrote:
On Thu, 2014-04-10 at 11:11 +0200, Sumit Bose wrote:
On Thu, Apr 10, 2014 at 10:34:10AM +0200, Pavel Reichl wrote: > On Wed, 2014-04-09 at 21:19 +0200, Sumit Bose wrote: >> On Wed, Apr 09, 2014 at 07:27:28PM +0200, Pavel Reichl wrote: >>> On Mon, 2014-04-07 at 18:13 +0200, Sumit Bose wrote: >>>> On Mon, Apr 07, 2014 at 02:22:04PM +0200, Pavel Reichl wrote: >>>>> Hello, >>>>> >>>>> I noticed these two warnings in clang. >>>>> >>>>> It would be great if the 2nd patch could be checked by Sumit to make >>>>> sure that the return value wasn't ignored on purpose. >>>> yes, I would prefer to ignore errors here. There might be various cases >>>> were we are not able to resolve a single SID but still can proceed with >>>> the others. >>> Please see attached patch. Feel free to NACK it, if you think it's more >>> pain than gain. >> Why not do something useful to avoid the compiler warning and print a >> SSSDBG_TRACE_ALL debug message with the returned error code in the case >> of an error? > Hello Sumit, > > I just took your previous response too literally - "I would prefer to > ignore errors here" - my bad. (Hopefully final) patch attached. Thank you. ACK
I wonder if the explicit (unsigned int) casts are needed to avoid warnings or if you are just calling them to be on the safe side?
Honestly, I just copy&past them. When I removed them I didn't notice any
If you copy&past this code then explicit casting is in two places. In future, it can in 3, 4, 5 ...
relevant clang or gcc warnings. I asked Jakub and he advised me that I could have used formatting macros PRIu16 and PRIu32.
I would suggest to leave patch as is for now.
Is it a problem to send another patch?
LS
If you have time to convert the debug messages, then by all means, send a patch. Maybe this could be a nice intern task? :-)
What about this patch?
LS
Your patch definitely fixes clang warning. I studied the patch and nearby code and it seems to me that we are not breaking anything.
It would be great to refactor this function soon as I personally find it really hard to understand.
ACK
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Aug 26, 2014 at 05:43:16PM +0200, Pavel Reichl wrote:
Your patch definitely fixes clang warning. I studied the patch and nearby code and it seems to me that we are not breaking anything.
It would be great to refactor this function soon as I personally find it really hard to understand.
ACK
Please file a ticket or else we'll forget.
On 08/26/2014 05:47 PM, Jakub Hrozek wrote:
On Tue, Aug 26, 2014 at 05:43:16PM +0200, Pavel Reichl wrote:
Your patch definitely fixes clang warning. I studied the patch and nearby code and it seems to me that we are not breaking anything.
It would be great to refactor this function soon as I personally find it really hard to understand.
ACK
Please file a ticket or else we'll forget.
OK.
https://fedorahosted.org/sssd/ticket/2417
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Tue, Aug 26, 2014 at 05:43:16PM +0200, Pavel Reichl wrote:
If you have time to convert the debug messages, then by all means, send a patch. Maybe this could be a nice intern task? :-)
What about this patch?
LS
Your patch definitely fixes clang warning. I studied the patch and nearby code and it seems to me that we are not breaking anything.
It would be great to refactor this function soon as I personally find it really hard to understand.
ACK
Pushed to master as 72db1f3ce67b0634f2843088f1198b3b350b72ab
Can you also provide a version that applies cleanly on top of sssd-1-11 ?
On (26/08/14 19:15), Jakub Hrozek wrote:
On Tue, Aug 26, 2014 at 05:43:16PM +0200, Pavel Reichl wrote:
If you have time to convert the debug messages, then by all means, send a patch. Maybe this could be a nice intern task? :-)
What about this patch?
LS
Your patch definitely fixes clang warning. I studied the patch and nearby code and it seems to me that we are not breaking anything.
It would be great to refactor this function soon as I personally find it really hard to understand.
ACK
Pushed to master as 72db1f3ce67b0634f2843088f1198b3b350b72ab
Can you also provide a version that applies cleanly on top of sssd-1-11 ?
I realized this problem is not in sssd-1-11 branch.
It was caused by commit 8a8618717c99b7331125fa736b45d9155da797d3, Author: Jakub Hrozek jhrozek@redhat.com Date: Tue Mar 11 17:39:31 2014 +0100
Only check GID if ID-mapping
And this commit was not backported to 1-11 branch.
LS
sssd-devel@lists.fedorahosted.org