On Thu, Dec 12, 2013 at 11:46:01AM +0100, Pavel Březina wrote:
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.
I guess it's a matter of taste :-), nevertheless ACK.
bye,
Sumit
>
>>+ 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
>