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

Sumit Bose sbose at redhat.com
Thu Dec 12 11:11:00 UTC 2013


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

> 
> >
> >>+    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
> >
> 



More information about the sssd-devel mailing list