[SSSD] [PATCHES] libsss_idmap enhancements
Jakub Hrozek
jhrozek at redhat.com
Fri Jun 28 14:48:03 UTC 2013
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.
[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.
[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;
+ }
+
[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.
[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.
More information about the sssd-devel
mailing list