On Sun, Jan 13, 2013 at 11:30:16PM +0100, Jakub Hrozek wrote:
Patch 0002: Refactor sysdb initialization
Ack with a suggestion:
The sysdb initialization looks cleaner to me now. I tested
sysdb clean initialization and upgrade and both look fine to me. While
you are touching the sysdb init, what about removing the "alt_db_path"
parameter of sysdb_init? It is not used anywhere and I can't think of a
meaningful way to use it.
We agreed to solve this in a separate patch:
https://fedorahosted.org/sssd/ticket/1765
Patch 0003: Refactor single domain initialization
Ack. This matches the intent of the refactoring.
Patch 0004: Remove the sysdb_ctx_get_domain() function.
Ack.
Patches 0005-0011: Make sysdb_*_dn() require a domain explictly
Ack. sysdb_netgroup_base_dn() was not covered with unit test, so I wrote
one,
Patch 0012: Move range objects into their own top-level tree.
Ack from my point of view, but Sumit would be a better authority on the
subdomains design.
I asked Sumit on IRC and he's fine with this, too.
Patch 0013: Pass domain to sysdb_get<pw/gr>nam() functions
This one is bigger and more involved than the rest of refactoring
patches. I think it would be nice to have a separate macro like
"domain_has_subdomain" that would be more self-explanatory than
"if (domain->parent && domain->fqnames)".
Let's solve this separately, too:
https://fedorahosted.org/sssd/ticket/1766
I think the ENOMEM part is wrong (a leftover probably):
+ if (domain->parent && domain->fqnames) {
+ ret = ENOMEM;
+ src_name = talloc_asprintf(tmp_ctx, domain->names->fq_fmt,
+ name, domain->name);
+ } else {
Simo explained the code to me on IRC and it's correct, I just didn't
read carefully enough. The ENOMEM would be used if we jump to a done
handler if the allocation fails.
One thing that surprised me is dom->fqnames being set to true now.
I think
I would expect it to be true even before the patches? I know it's
mentioned in the commit message, I'm just wondering if the code worked
by accident until now?
Other than that, retrieving users and groups by name worked for me for
both native and trusted users and groups.
Patches 0014-0015: Pass domain to sysdb_get<pwu/grg><id() functions
Ack. Code looks fine, is covered by tests and getting both trusted and
native users and groups by ID works fine.
Enumeration also works fine.
Patch 0016: Add domain option to sysdb_get/netgr/attrs() fns
Ack. Code is covered, looks sane, retrieving and updating netgroups works fine.
Patch 0017: Add domain argument to sysdb_initgroups()
Ack. Code is covered, looks sane, user login works fine. I was suprised that
sysdb_initgroups, in spite of being one of the most important sysdb
functions doesn't have a unit test, so I wrote one.
Patch 0018: Add domain argument to sysdb_get_user_attr()
Ack. Code is covered, looks sane and authentication still works and sets the
lastOnlineAuth correctly.
Patch 0019-0022: Add domain to sysdb_search_user/group_by_name/uid/gid
Ack. Code is covered. I tested the basic ID operations (get{pw,gr}{uid,nam}),
and also some basic HBAC and SELinux sanity tests.
Patch 0023: Add domain arg to sysdb_search_netgroup_by_name
Ack. Netgroups still seem to work, code is covered and looks sane
Patch 0024-0027: Add domain argument to sysdb_set_user/group/netgroup_attr()
The last patch is misnamed, it should say "netgroup", not "group".
Otherwise Ack, code is covered, looks sane.
I'll rename the patch before pushing.
Patch 0028: Add domain argument to sysdb_get_new_id()
Ack. Local domain tools still work OK. I added a sanity test to the test
suite to cover the code.
Patch 0029-0031: Add domain argument to sysdb_add{,_basic}_user()
Ack. Retrieving users and groups still works, the code is mostly covered.
Patch 0032: Add domain arguments to sysdb_add_inetgroup fns
Ack, apart from typo in the commit message. Code is covered and
netgroups still seem to work (both LDAP and IPA).
I'll fix the typo before pushing.
Patch 0033-0034: Add domain argument to sysdb_store_user/group
Ack. Saving users and groups still work, code looks OK and is covered.
Patch 0035: Add domain arg to sysdb group member functions.
Ack. There's some room for improving the code coverage, but adding both
users and groups as members works.
Patch 0036-0037: Add domain argument to sysdb_cache_passwd/auth
Ack. Code is covered and cached auth still works.
Patch 0038-0040: Add domain argument to sysdb_store/search/delete_custom
Ack. Code is covered and functionality that uses the custom tree, such
as HBAC, still works.
That leaves less than 20 patches to go. I'll review them tomorrow.