On Fri, Oct 25, 2013 at 12:06:25PM +0200, Sumit Bose wrote:
> On Fri, Oct 25, 2013 at 11:21:28AM +0200, Jakub Hrozek wrote:
> > 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 <sbose(a)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 <sbose(a)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?
>
> There was no patch but I changed version-info to 2:0:2. Is this what you
> would suggest?
>
> >
> > > From dffdd7cc99025cb8790f34f79bdf294762f3468d Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose <sbose(a)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 <sbose(a)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.
>
> good catch, fixed.
>
> >
> > > + 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..
>
> fixed.
>
> Thank you for the review, new versions attached.
>
> bye,
> Sumit
ACK to these patches.