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

Jakub Hrozek jhrozek at redhat.com
Thu Dec 12 12:08:11 UTC 2013


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?= <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.
> 
> 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) {
        ^~~~~~~~~~~~~~



More information about the sssd-devel mailing list