On Fri, Jan 11, 2013 at 10:46:57AM -0500, Simo Sorce wrote:
New rebase pushed
Thank you, this patchset compiles and all tests pass. I don't think it is
realistic to review every codepath in every patch, it's too big. I reviewed
the domain initialization carefully, but then many patches "just" add the
domain parameter to a sysdb function and adjust the callers. So I used gcov
to see code coverage of the changed functions and read the code, but not
that carefully as when reviewing a regular patch, if the code was covered.
I attached the gcov output after my review of the first part of this
patchset and some more unit tests. Please squash the unit tests into the
approproate patches or simply include them on top, whatever suits you
better.
tl;dr version - it's mostly good. I only have some suggestions, nothing
blocking the push. Given the sheer size of the patchset, I'd say that we
should push the patchset and fix the remaining issues in separate
tickets/patches to spare us from rebasing the whole thing when the whole
patchset is reviewed.
Patch 0001: The Big sysdb/domain split-up!
Ack
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.
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.
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)".
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 {
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.
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).
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.