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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel