On Thu, Oct 24, 2013 at 02:14:59PM +0200, Sumit Bose wrote: > Hi, > > this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 . > Currently we rely on the fact that the external mapping is the default > in the case of an error. But this leads to a loot of useless CPU cycles > and maybe irritating error messages. With this patch set each trusted AD > member domain gets a proper range assigned if the forest root is > configured to manage the POSIX attributes. > > bye, > Sumit
Hi, I only have two minor comments, see below: > From e1a86771137f67e0e8d24c4bbb014bfc21d85538 Mon Sep 17 00:00:00 2001 > From: Sumit Bose <sb...@redhat.com> > Date: Wed, 23 Oct 2013 14:39:55 +0200 > Subject: [PATCH 1/4] find_subdomain_by_sid: skip domains with missing > domain_id ACK. Thanks for the unit test. > From dc0115802904c954c79d90cd846da9f5bd965019 Mon Sep 17 00:00:00 2001 > From: Sumit Bose <sb...@redhat.com> > Date: Thu, 24 Oct 2013 11:44:11 +0200 > Subject: [PATCH 2/4] idmap: add > sss_idmap_domain_by_name_has_algorithmic_mapping() The code itself is good, but we need to bump the version number. See the attached patch, can you squash it in if you agree? > From dffdd7cc99025cb8790f34f79bdf294762f3468d Mon Sep 17 00:00:00 2001 > From: Sumit Bose <sb...@redhat.com> > Date: Thu, 24 Oct 2013 11:45:57 +0200 > Subject: [PATCH 3/4] sdap_idmap_domain_has_algorithmic_mapping: add domain > name argument ACK > From 3e7283a39994ac24a9ff9dfede0d735cc89b4bb0 Mon Sep 17 00:00:00 2001 > From: Sumit Bose <sb...@redhat.com> > Date: Wed, 23 Oct 2013 13:30:57 +0200 > Subject: [PATCH 4/4] IPA: add trusted domains with missing idrange > > If the forest root of a trusted forest is managing POSIX IDs for its > users and groups the same is assumed for all member domains in the > forest which do not have explicitly have an idrange set. > > To reflect this SSSD will create the matching ranges automatically. One nitpick: > + tmp_ctx = talloc_new(NULL); > + if (tmp_ctx == NULL) { > + DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n")); > + return ENOMEM; > + } > + > + for (c = 0; c < range_count; c++) { > + r = range_list[c]; > + if (r->trusted_dom_sid != NULL > + && strcmp(r->trusted_dom_sid, forest_root->domain_id) == 0) { > + > + if (r->range_type == NULL > + || strcmp(r->range_type, IPA_RANGE_AD_TRUST_POSIX) != 0) > { > + DEBUG(SSSDBG_MINOR_FAILURE, > + ("Forest root does not have range type [%s].\n", > + IPA_RANGE_AD_TRUST_POSIX)); we would leak tmp_ctx here, we should goto done instead. > + return EINVAL; > + } > + > + range.min = r->base_id; > + range.max = r->base_id + r->id_range_size -1; > + range_id = talloc_asprintf(tmp_ctx, "%s-%s", dom_sid_str, > r->name); > + if (range_id == NULL) { > + DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n")); > + ret = ENOMEM; > + goto done; > + } > + > + err = sss_idmap_add_domain_ex(idmap_ctx->map, dom_name, > dom_sid_str, > + &range, range_id, 0, true); > + if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) { > + DEBUG(SSSDBG_CRIT_FAILURE, ("Could not add range [%s] to ID > map\n", > + range_id)); If you're changing the patch, can you also reflow this line? It's past 80 characters and my editor highlights the line.. > + ret = EIO; > + goto done; > + } > + > + found = true; > + } > + } _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel