[SSSD] [PATCHES] ad: use tokengroups even when id mapping is disabled

Pavel Březina pbrezina at redhat.com
Thu Dec 12 10:46:01 UTC 2013


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 at 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ad-refactor-tokengroups-initgroups.patch
Type: text/x-patch
Size: 27787 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131212/76efb212/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-ad-use-tokengroups-even-when-id-mapping-is-disabled.patch
Type: text/x-patch
Size: 23164 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131212/76efb212/attachment-0001.bin>


More information about the sssd-devel mailing list