[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