On Sun, Jan 13, 2013 at 11:30:16PM +0100, Jakub Hrozek wrote:
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.
Short version - please squash in or apply on top the attached unit
tests.
- Why was sysdb_idmap_dn made non static?
The rest looks good. See below for more information.
Patch 0041: Add domain arg to sysdb_search_users()
Ack. I added a unit test as there was none in sysdb tests.
Patch 0042: Add domain argument to sysdb_delete_user()
Ack, code looks sane, is covered. I tested by looking up a user I
removed from the server previously.
Patch 0043: Add domain argument to sysdb_search_groups()
Ack. I added a unit test as there was none in sysdb tests.
Patch 0044: Add domain argument to sysdb_delete_group()
Ack, code looks sane, is covered. I tested by looking up a group I
removed from the server previously.
Patch 0045: Add domain arg to sysdb_search/delete_netgroup()
Ack, code is covered by tests, retrieving netgroups still works.
Patch 0046: Add domain argument to sysdb_has/set_enumerated
Ack. Code is tested and enumeration still works.
Patch 0047: Add domain argument to sysdb_remove_attrs()
Ack. I added a unit test and tested with service records as well.
Patch 0048: Add domain argument to sysdb_idmap_ funcitons
Why did you make sysdb_idmap_dn non static? The code needs tests, but
that's outside the scope of the review. AD provider smoke test went OK.
Patch 0049: Add domain arguemnt to sysdb_get_real_name()
Ack by code inspection only this time. I don't have any LDAP server with
aliases.
Patch 0050: Add domain argument to sysdb autofs functions
Ack. Code is tested and basic autofs sanity test went OK.
Patch 0051: Add domain argument to sysdb selinux functions
Ack. There are no unit tests, but my selinux test scripts passed and the
code looks good.
Patch 0052: Add domain arguments to sysdb services functions
Ack. I only tested services with the proxy back end, but I think that's
good enough. They also have a unit test.
Patch 0053: Add domain arguments to sysdb ssh functions
Ack. Covered by unit tests and the CLI still works fine.
Patch 0054: Add domain arguments to sysdb sudo functions
Ack. Code needs coverage but basic sanity sudo test went fine.
Patch 0055: Add domain to some subdomain functions
Ack. I tested some basic ID operations in a trusted domain and they
worked OK.
Patch 0056: Pass the domain to upgrade functions
Visual ack. I only tested upgrade from 0.14 to 0.15 but also this
patch looks good to me.
Patch 0057: Move mpg flag to the domain where it belongs
Ack. The local domain still worked fine for me.
Patch 0058: Kill sysdb->domain
Ack
Patch 0059: Stop creating fake sysdb contexts
Ack from my point of view, but Sumit is a better authority on
subdomains. But ID operations in a trusted domain still worked.