On Thu, Dec 12, 2013 at 12:11:00PM +0100, Sumit Bose wrote: > 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
I see a number of compilation warnings with the patches (using clang-3.3) /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:9: warning: variable 'in_transaction' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (ret != EOK) { ^~~~~~~~~~ /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:873:9: note: uninitialized use occurs here if (in_transaction) { ^~~~~~~~~~~~~~ /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:785:5: note: remove the 'if' if its condition is always false if (ret != EOK) { ^~~~~~~~~~~~~~~~~ /home/remote/jhrozek/devel/sssd/src/providers/ldap/sdap_async_initgroups_ad.c:778:9: warning: variable 'in_transaction' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (groups == NULL) { ^~~~~~~~~~~~~~ _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel