This is not (yet) a review. Just replying to a few questions in the
email, inline.
On Mon, 2012-04-02 at 10:07 +0200, Jan Zeleny wrote:
...
> 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.
Hm, I didn't write the routine and I could read it quite well. Nevertheless I
added some comments. Those routines functions you are missing would be rather
simple (almost aliases), that's why they are missing. Do you think there is a
need for them?
When dealing with Active Directory directly, it may be possible that we
will only discover a new domain by requesting it the first time (as
opposed to acquiring a complete list ahead of time). If that's the case,
it would be handy to have a routine to simply add one to the list. I
agree that removal is probably unnecessary in most situations.
> The creation of new_sd_list should be its own function.
I disagree. The funcion would be called on this one place anyway so there is
probably only a little gain in doing that.
The gain is mostly in readability. The creation of the new_sd_list is
moderately complex and slightly distracts from the main purpose of the
function. It's not worth nacking on, just my preference. Keep it if you
wish.
...
> 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'm not sure what do you mean here, how could it be null when it's not a
pointer?
As Jakub noted in his review, I meant to say
becli->bectx->bet_info[BET_SUBDOMAINS].bet_ops here.
> One additional point about this patch. I don't like that
you're forcing
> the BET_SUBDOMAINS to get its data from the ID provider. I'd much prefer
> that we add a subdomain_provider option, which defaults to being the
> same as the ID provider. I prefer to maintain the distinction in case we
> ever decide to have alternate mechanisms for determining the subdomains
> (in IPA or AD).
I discussed this with Sumit to be sure and we agreed that subdomains are
supposed to be tightly bound to the ID provider. One of the reasons is that ID
provider has to sort out all following requests so it will also needs to keep
track of all subdomains.
Sure, I'm fine with that, then.
...
> I'd prefer if you added an ipa_subdomain_map and used that
for
> determining the attribute names to use in
> ipa_subdomains_parse_results(). Mostly for consistency.
I don't think there would be much gain, the code almost wouldn't change, only
names of attributes would be different.
Again, code-readibility is a good thing. I'd like to have a simple map
that we can glance at.
> > #0011
> > New ID-related config options for subdomains, these have to be present
> > because IPA provider doesn't provide these values and defaults need to
> > be implemented. Having defaults on the responder level didn't seem right
> > since the policy might differ for each domain.
>
> Nack.
>
> I don't think this really makes sense at all. In most cases, users will
> prefer to use the value on the LDAP server. If they choose to override
> it, they'll do so through the existing override options (in the case of
> override_homedir, it already has %d available anyway.
>
> We definitely don't need separate handling for shells. I can *kind of*
> see a value if you wanted to have only subdomains have a non-default
> location. I'm not sure I like that though. I feel like it's probably
> more complexity than we need.
I think you possibly missed the point. The point is that this information is
NOT on the server, therefore we need a value that will fill it in. Otherwise
only a blank field will be stored in sysdb and returned to the client
Ok, I think I understand now, but the manpages need to be MUCH more
clear. It sounds like you're adding this option to always override
subdomain home directory values. Please clarify the documentation.
I still don't see a use for the shells though. The OS already handles
this internally by translating a NULL value for the shell into "the
system default shell" (usually /bin/sh). This is handled by glibc and
isn't our concern.