On Tue, 2012-03-20 at 17:42 +0100, Jan Zeleny wrote:
Hi guys,
it took me and Sumit some time but we finally have completed the first stage
of support for subdomains. I'm sending all patches in attachment.
This stage has basic support for subdomains but some pieces like PAC support
are left out. We agreed that those can wait for the second stage which depends
on some server side changes that are not ready yet. We'll keep you posted.
Please note that patches depend on Sumit's previous idmap library patch. I'll
get that reviewed either today or tomorrow.
#0001
All sysdby routines that are used in later patches
Nack.
Please fix debug levels to use the new format.
Why are you creating a NULL-terminated array *and* returning the number
of elements? Usually you would do only one or the other. This isn't the
nack, just a curiosity.
I don't care for the inner loop through the elements in
sysdb_get_subdomains(). I'd rather see you use
ldb_msg_find_attr_as_string() three times. The benefit here is that you
can easily detect if one or more the elements are missing. In the
current approach, you'll just iterate through, but if the DN existed but
had no attributes associated with it, you would be left with a 'struct
subdomain_info' that contained only NULL strings... a recipe for
disaster.
Though, if you want to be *really* paranoid, you might even want to use
ldb_message_find_element() so you can test that they are in fact
single-valued as well.
sysdb_update_subdomains() desperately needs comments. It took me ten
minutes to figure out the logic there. I also question why you didn'd
include sysdb_[add|remove]_subdomain(). An explanation of this lack
should be in the source code, if there's a good reason.
The tests added by this patch depend on routines added in Patch 0003
(domain_info_utils.c). Probably this should be reordered.
#0002
Responder routines for asking provider for a list of supported subdomains
Nack.
dp_get_domains_callback() should be static.
Please add comments explaining the purpose of the "hint" argument.
Please redesign this routine to use the common sss_dp_issue_request().
This will give us a few things: 1) Bundling of concurrent 'force'
lookups, 2) processing closer to the tevent_req style.
get_domains_send() must have a matching get_domains_recv() function for
consumers to retrieve, if only to be able to find out if there were
errors.
Take a look at how sss_dp_get_account_send() and
sss_dp_get_sudoers_send() works now with sss_dp_issue_request()
If parsing fails in dp_get_domains_callback(), you're jumping to done:
rather than just treating it like other errors.
This patch also depends on domain_info_utils.c
get_domains_done() needs comments on the logic.
The creation of new_sd_list should be its own function.
#0003
The routine responder_get_domain is modified to look for subdomains as well
when looking for the correct domain descriptor
Nack
As mentioned above, domain_info_utils.c should appear earlier in the
patch series. Probably it should have its own patch.
The logic in this patch looks good to me.
#0004
If a request for fully qualified entity (user/group) comes to the responder
and its domain has not been found, it updates its list of domains and
supported subdomains in each domain. To update the list, a get_domains call is
sent to each provider.
Nack
This is going to need quite a bit of modification to handle the changes
required for Patch 0002. With that in mind, the logic looks fine.
#0005
Create a list of all domains and subdomains when the first request after start
comes in. To create the list, a get_domains call is sent to each provider.
Nack
Similar to 0004, this is going to need to be reworked to account for
changes to 0002.
I don't understand why the addition of "if (dctx->domname)" in the
callbacks wasn't part of patch 0004. Also, a comment would got far
towards explaining why that's being done (I assume it's to handle
explicit domain requests)
#0006
Check sub-domains in nss_cmd_get{pwuid|grgid}_search()
Nack
Please add comments to get_next_dom_or_subdom() to explain the logic.
The code itself looks good.
#0007
Basic infrastructure for handling get_subdomains requests in providers.
Nack
Please fix debug levels to use the new format.
You initialized be_req to NULL twice.
You need to check for whether becli->bectx->bet_info[BET_SUBDOMAINS] is
NULL, or else you'll crash on domains that don't support subdomain
lookups.
I'll get to the rest of the patches a bit later.