[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