On 12/10/2013 04:59 PM, Sumit Bose wrote:
On Wed, Nov 20, 2013 at 02:41:49PM +0100, Pavel Březina wrote:
> On 11/19/2013 11:52 AM, Jakub Hrozek wrote:
>> On Fri, Nov 15, 2013 at 12:22:53PM +0100, Pavel Březina wrote:
>>>
https://fedorahosted.org/sssd/ticket/1568
>>
>>> From b66343b207679cbbbdb5d4a54a7f465fbf2ec97f Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina(a)redhat.com>
>>> Date: Tue, 12 Nov 2013 13:52:35 +0100
>>> Subject: [PATCH 1/3] sysdb: add sysdb_group_dn_str()
>>
>> The patch doesn't apply, but more importantly there already is
>> sysdb_group_dn_str(), can you use that instead?
>
> Thanks.
>
>>
>> The other two patches don't apply either. I have just a couple of
>> comments and questions as I scrolled through them:
>>
>> Patch #2 looks good to me so far, but I only read the diff.
>>
>> In patch #3, I don't see the point of using tmp_ctx and stealing to
"state"
>> later if the request is relatively short. Can you use "state"
directly?
>> That way you guarantee that the memory will go away with the request.
>
> I don't know, I think freeing all resources when they become
> irrelevant is more clear.
>
>> btw did you test if the code works even if only some of the groups have
>> posix attributes set?
>
> Yes, those non-posix groups are downloaded by SID but they have
> empty members.
>
> Rebase patches are attached.
Patches apply and work as expected. Nevertheless I have two comments.
>
> -errno_t
> -sdap_get_ad_tokengroups_initgroups_recv(struct tevent_req *req)
> +int sdap_ad_tokengroups_initgroups_recv(struct tevent_req *req)
> {
is there a reason to switch from errno_t to int here?
None. Fixed.
> +struct tevent_req *
> +sdap_ad_tokengroups_initgroups_send(TALLOC_CTX *mem_ctx,
> + struct tevent_context *ev,
> + struct sdap_id_ctx *id_ctx,
> + struct sdap_options *opts,
> + struct sysdb_ctx *sysdb,
> + struct sss_domain_info *domain,
> + struct sdap_handle *sh,
> + const char *name,
> + const char *orig_dn,
> + int timeout,
> + bool use_id_mapping)
> +{
> + struct sdap_ad_tokengroups_initgroups_state *state = NULL;
> + struct tevent_req *req = NULL;
> + struct tevent_req *subreq = NULL;
> + errno_t ret;
> +
> + req = tevent_req_create(mem_ctx, &state,
> + struct sdap_ad_tokengroups_initgroups_state);
> + if (req == NULL) {
> + DEBUG(SSSDBG_CRIT_FAILURE, ("tevent_req_create() failed\n"));
> + return NULL;
> + }
> +
> + state->use_id_mapping = use_id_mapping;
> +
> + if (state->use_id_mapping) {
> + subreq = sdap_ad_tokengroups_initgr_mapping_send(state, ev, opts,
> + sysdb, domain, sh,
> + name, orig_dn,
> + timeout);
> + } else {
> + subreq = sdap_ad_tokengroups_initgr_posix_send(state, ev, id_ctx, opts,
> + sysdb, domain, sh,
> + name, orig_dn,
> + timeout);
> + }
I think it would avoid a bit of code duplication if this check would be
done after the tokenGroups is read from the server.
You would avoid a little code duplication, but I believe it is not worth
it, because the duplication is only in calling
sdap_get_ad_tokengroups_send() and the amount of glue code would be
approximately the same.
Also I think, the current code flow is easier to follow.
> + if (subreq == NULL) {
> + ret = ENOMEM;
> + goto immediately;
> + }
> +
bye,
Sumit
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel