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?= <pbrez...@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@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