The mails grow huge. I'm going to trim the response a little and omit patches
that were already acked elsewhere.
Patch 0001: Ack
Patch 0002: Ack
Patch 0003: Ack
Patch 0004: Ack
Patch 0005: Ack
On Fri, Apr 06, 2012 at 01:52:36PM +0200, Jan Zelený wrote:
> > > Patch 0006: Retrieve subdomains if there is a request
for fully
> > > qualified user
> > > Looks OK, will just need amendment when sss_dp_get_domains_recv() is
> > > implemented. Currently the patch supports users and netgroups, are
> > > there plans to also support services and the SSH responder? I think we
> > > can leave out the sudo responder for now, sudo doesn't support
domains
> > > (yet?). Autofs in theory can send a request for a fully-qualified map,
> > > but it doesn't so the work can be done later, although I would prefer
> > > a ticket to show that we are aware of the issue.
> >
> > The amendment has been done.
>
> No, sss_dp_get_domains_recv() is not used anywhere.
Fixed
> > About support of another responders - I honestly
> > don't know. I wasn't aware that a request for fully qualified
service/ssh
> > entity can be given to SSSD. Perhaps Sumit will know more.
>
> Replied elsewhere.
Fixed
Please also remove "subreq" from setnetgrent_send() and the declaration
of setnetgrent_cb().
Patch 0007: Ack
Patch 0008: Ack
Patch 0009: Ack
Patch 0010: Ack
Patch 0011: Ack
> > > Patch 0012: New config options for subdomains
> > > See my reply in the other part of this thread. If you are goig forward
> > > with these options, add them to the configAPI.
> >
> > Based on previous discussion I removed the part which is setting default
> > shell. I intend to keep the homedir for two reasons:
> >
> > I recalled why I created the config option in the first place. It's
> > because override_homedir can't be set per domain. After your comment
> > yesterday, I checked to be sure and IMO the man page is incorrect and
> > the override_homedir only works for responder.
>
> What was your testcase and what exactly wasn't working for you?
>
> The option works fine for me in a subdomain and the code seems to be quite
> clear (get_homedir_override())
I know it is straightforward, but it didn't work for me.
Can you file a bug if the override is not working in a domain? It
should and if something is not clear, then it's most probably a docs
bug..
However when looking
a bit deeper I discovered a bug in completely different part of the patch set.
This is now fixed and the override works as expected.
> > The second reason is that admins might want to separate homedirs for
> > native users from homedirs for trusted users on some systems.
>
> Then let's extend the homedir template by a subdomain name or "full
> domain name" option.
I'm not sure about this. For one thing this functionality is already there.
But I was considering scenario when the path to subdomain homedirs is
completely different. Something like this:
/home/ipa/%u - the main IPA domain
/home/ad/%u - AD subdomains
I there is a consensus that it is not necessary, I can remove it. But frankly
I can see us having exactly this RFE several months later.
If the AD users are to be treated differently than native IPA users the
new option is OK.
Patch 0013: Ack
> > > Patch 0014: Add s2n extended operation
> > > Old DEBUG macros again. There is a typo in s2n_response_to_attrs, one
> > > of the debug messages says "go [%s]", should be "got
[%s]". Otherwise
> > > looks fine.
> >
> > Done
>
> GCC emits these warnings when compiling with -O2:
> src/providers/ipa/ipa_s2n_exop.c: In function 'ipa_s2n_get_user_done':
> src/providers/ipa/ipa_s2n_exop.c:408:16: warning: 'retoid' may be used
> uninitialized in this function [-Wuninitialized]
> src/providers/ipa/ipa_s2n_exop.c:582:11: note: 'retoid' was declared
> here
> src/providers/ipa/ipa_s2n_exop.c:408:35: warning: 'retdata' may be used
> uninitialized in this function [-Wuninitialized]
> src/providers/ipa/ipa_s2n_exop.c:583:20: note: 'retdata' was declared
> here
> src/providers/ipa/ipa_s2n_exop.c: In function
> 'ipa_s2n_get_acct_info_send':
> src/providers/ipa/ipa_s2n_exop.c:84:9: warning: 'bv_req' may be used
> uninitialized in this function [-Wuninitialized]
> src/providers/ipa/ipa_s2n_exop.c:532:20: note: 'bv_req' was declared
> here
All fixed
Ack
Patch 0015: Ack
Patch 0016: Ack
Patch 0017: Ack
Patch 0018: Ack
Patch 0019: Ack
Patch 0020: Ack
Patch 0021: Ack
Becuse the only remaining review issue is a trivial one and I know that
Stephen has been reviewing the patches as well, feel free to resend
another patch set when/if Stephen sends his comments.
Great work!