On 11/23/2015 05:35 PM, Pavel Reichl wrote:
On 11/17/2015 11:51 AM, Sumit Bose wrote:
>
> From 14ad252d2715e5eb1eed139d4497fc0155c51f85 Mon Sep 17 00:00:00 2001
> From: Sumit Bose<sbose(a)redhat.com>
> Date: Fri, 13 Nov 2015 12:15:52 +0100
> Subject: [PATCH 2/2] initgr: only search for primary group if it is not
> already cached
>
> Related
tohttps://fedorahosted.org/sssd/ticket/2868
> ---
> src/providers/ldap/sdap_async_initgroups.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/src/providers/ldap/sdap_async_initgroups.c
b/src/providers/ldap/sdap_async_initgroups.c
> index
e9fcb272d3e906555c63d51e0aab90d1f575f3f8..2c25505753c0705649a70bb7ee3ab0dd42d2c944 100644
> --- a/src/providers/ldap/sdap_async_initgroups.c
> +++ b/src/providers/ldap/sdap_async_initgroups.c
> @@ -3065,6 +3065,7 @@ static void sdap_get_initgr_done(struct tevent_req *subreq)
> char *dom_sid_str;
> char *group_sid_str;
> struct sdap_options *opts = state->opts;
> + struct ldb_message *msg;
>
> DEBUG(SSSDBG_TRACE_ALL, "Initgroups done\n");
>
> @@ -3182,14 +3183,25 @@ static void sdap_get_initgr_done(struct tevent_req *subreq)
> goto fail;
> }
>
> - subreq = groups_get_send(req, state->ev, state->id_ctx,
> - state->id_ctx->opts->sdom, state->conn,
> - gid, BE_FILTER_IDNUM, BE_ATTR_ALL, false, false);
> - if (!subreq) {
> - ret = ENOMEM;
> - goto fail;
> + ret = sysdb_search_group_by_gid(tmp_ctx, state->dom, primary_gid, NULL,
> + &msg);
> + if (ret == EOK) {
> + DEBUG(SSSDBG_TRACE_FUNC,
> + "Primary group already cached, nothing to do.\n");
> + talloc_free(tmp_ctx);
Do you think it would be bad idea to remove this line
> + tevent_req_done(req);
> + return;
and this?
I think we can reuse 'free' and 'return' that is executed right before
'fail' namespace. It would be nice to avoid adding extra return.
> + } else {
would you also consider moving
gid = talloc_asprintf(state, "%lu", (unsigned long)primary_gid);
if (gid == NULL) {
ret = ENOMEM;
goto fail;
}
into this branch only?
>> + subreq = groups_get_send(req, state->ev, state->id_ctx,
>> + state->id_ctx->opts->sdom,
state->conn,
>> + gid, BE_FILTER_IDNUM, BE_ATTR_ALL, false,
>> + false);
>> + if (!subreq) {
>> + ret = ENOMEM;
>> + goto fail;
>> + }
>> + tevent_req_set_callback(subreq, sdap_get_initgr_pgid, req);
>> }
>> - tevent_req_set_callback(subreq, sdap_get_initgr_pgid, req);
>>
>> talloc_free(tmp_ctx);
>> return;
>> -- 2.1.0
>>
>
> Hello I'm just starting to review the patch. I haven't tested the code yet
but it looks OK to me and CI passed. I have just a little proposal mentioned above.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org