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. _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel