Sumit sent me his set of patches last Friday. I've done some merges, filled
commit messages and other cosmetic adjustments. I've also done initial review
and I'm sending my comments:
get_rid() doesn't need to be in the header file, it can be static in the module
Defining idmap_talloc() and idmap_free() twice, please remove one of them.
The condition if (pac_ctx->idmap_ctx == NULL) in domsid_rid_to_uid() will be
always false, won't it? If it was NULL, the entire responder would fail way
earlier.
Missing debug_level initialization in main() in PAC responder
pac_save_memberships_next() is missing static in its definition (declaration is
ok)
pac_save_memberships_recv() is missing static
you are not assigning the result to *_pwd in get_pwd_from_pac()
Is the TODO in save_pac_user() valid and important? Do we need to address this
before pushing the patch set?
Nitpick: please use initialization instead of "g = 0;" in get_gids_from_pac()
Typo: Remote users are members ... (comment in pac_save_memberships_send())
Please remove the sysdb_search_group_by_gid() call in
pac_save_memberships_next(), it is redundant there and can be replaced by
checking the return code after calling pac_store_memberships().
You are not freeing pr_ctx in some error cases. The only place pr_ctx is
free'd is pac_save_memberships_done(), i.e. only when everything went ok.
Please add some talloc_free() calls where necessary. My first guess is that
pac_get_domains_done() and pac_add_pac_user() are the only two places where it
is necessary.
In function local_sid_to_id(): I think another check should be in place:
if (rid-remote_min+local_min > local_max) return ERANGE;
Sumit, please contact me if you need help with anything.
Thanks
Jan