On Fri, Jun 28, 2013 at 05:22:55PM +0200, Sumit Bose wrote:
On Fri, Jun 28, 2013 at 04:48:03PM +0200, Jakub Hrozek wrote:
> On Fri, Jun 28, 2013 at 12:38:50PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > the following 8 patches contain some enhancements for libsss_idmap which
> > will allow to handle external mapping as well and would make is possible
> > to use the calls from the library more generally in the LDAP based ID
> > providers.
> >
> > See commit messages for more details.
> >
> > bye,
> > Sumit
>
> [PATCH 1/8] idmap: allow first RID to be set
> I think we should amend the header file to make it clear that some
> functions can return IDMAP_COLLISION. But it's OK to do it later via a
> ticket, no need to delay the release.
done
>
> [PATCH 2/8] idmap: add optional unique range id
> Ack
>
> [PATCH 3/8] idmap: add option to indicate external_mapping
> Ack
>
> [PATCH 4/8] idmap: allow NULL domain sid for external mappings
> Ack
>
> [PATCH 5/8] idmap: add calls to check if ID mapping conforms to ranges
> Ack, but I will add a newline after line 587 to separate two function
> calls before pushing.
done
>
> [PATCH 6/8] idmap: add sss_idmap_domain_has_algorithmic_mapping
>
> I think in that in sss_idmap_domain_has_algorithmic_mapping you should
> return IDMAP_NO_DOMAIN here instead:
>
> +
> + if (ctx->idmap_domain_info == NULL) {
> + return IDMAP_SID_UNKNOWN;
> + }
fixed
> +
>
> [PATCH 7/8] Add sdap_idmap_domain_has_algorithmic_mapping()
> This is the patch that requires find_new_domain(). The code looks mostly
> good, but can you change the flow of the end of the function:
>
> + err = sss_idmap_domain_has_algorithmic_mapping(ctx->map, dom_sid,
&has_algorithmic_mapping);
> +
> + if (err == IDMAP_SUCCESS) {
> + return has_algorithmic_mapping;
> + } else {
> + return false;
> + }
> +}
>
> So that there is always a return outside if-else? Something like:
> -------------
> err = sss_idmap_domain_has_algorithmic_mapping(ctx->map, dom_sid,
&has_algorithmic_mapping);
> if (err == IDMAP_SUCCESS) {
> return has_algorithmic_mapping;
> }
>
> return false;
> -------------
>
> I find it much readable when there is an exit point and I think even our
> coding guidelines mandate that.
I will resend this patch later, I removed it from the updated versions
attached.
>
> [PATCH 8/8] Add cmocka based tests for libsss_idmap
> Usable even without patch #7, so Ack. I'm glad we keep adding unit
> tests, that's really good.
Thank you for the review.
bye,
Sumit
These patches look good to me, I also ran a couple of tests with the AD
provider to make sure the mapping will stay consistent with 1.10 and it
did.
So ACK