[SSSD] [PATCHES] Initial refactoring for #1747

Jakub Hrozek jhrozek at redhat.com
Tue Jan 15 09:54:07 UTC 2013


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.



More information about the sssd-devel mailing list