On Tue, Oct 29, 2013 at 12:37:11PM +0100, Sumit Bose wrote:
> On Tue, Oct 29, 2013 at 11:41:52AM +0100, Jakub Hrozek wrote:
> > On Tue, Oct 29, 2013 at 10:12:34AM +0100, Jakub Hrozek wrote:
> > > On Tue, Oct 29, 2013 at 09:43:08AM +0100, Sumit Bose wrote:
> > > > On Mon, Oct 28, 2013 at 10:28:07PM +0100, Jakub Hrozek wrote:
> > > > > On Mon, Oct 28, 2013 at 10:20:24PM +0100, Sumit Bose wrote:
> > > > > > On Mon, Oct 28, 2013 at 04:55:17PM +0100, Jakub Hrozek
wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > The first patch adds a unit test for getgrnam. I
wanted to make sure we
> > > > > > > don't break anything this close to the 1.11.2
release.
> > > > > > >
> > > > > > > The second patch is a workaround until
> > > > > > >
https://fedorahosted.org/sssd/ticket/2129 is fixed
properly.
> > > > > > >
> > > > > > > Consider a group entry such as:
> > > > > > > cn: subgroup@subdom
> > > > > > > ghost: someuser
> > > > > > > ghost: anotheruser@subdom
> > > > > > >
> > > > > > > Currently in order to print all group members as FQDN
(which is the default
> > > > > > > for AD provider), the code needs to iterate over the
ghost attributes and
> > > > > > > parse them into (name,domain) and optionally re-add
the domain.
> > > > > > >
> > > > > > > The proper fix would be to store always just the FQDN
in the hardcoded
> > > > > > > form of user@domain
> > > > > >
> > > > > > sorry, but nss-srv-tests fails for me after applying the
patch to master.
> > > > > > See
http://paste.fedoraproject.org/50054/13829949/ for the
short and
> > > > > >
http://paste.fedoraproject.org/50056/29949861/ for the full
debug
> > > > > > output.
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > > >
> > > > > I should have explicitly said that these patches must be applied
on top
> > > > > of those in "[PATCH] ad: support cross domain
membership", do they still
> > > > > fail for you?
> > > >
> > > > I'm sorry but yes. Am I missing any other patch? This is the top
of the
> > > > branch I test with on a 32bit system:
> > > >
> > > > d703220 NSS: Print FQDN for groups with mixed domain membership
> > > > de24e2d TEST: Test getgrnam with emphasis on members
> > > > bdd6b7e NSS: Fix parenthesis
> > > > 267adcb LDAP: Check all search bases during nested group processing
> > > > b0fc582 nested groups: pick correct domain for cache lookups
> > > > 0335a23 sdap_fill_memberships: pick correct domain for every member
> > > > 1e3112f ghosts: pick correct domain for every member
> > > > 8c6d1a4 sdap: add sdap_domain_get_by_dn()
> > > > c6360d8 sdap: store base dn in sdap_domain
> > > > 3226d5b ad: shortcut if possible during get object by ID or SID
> > > > da34cf4 ad: destroy ptasks when subdomain is removed
> > > > 44e8e96 ipa: destroy cleanup task when subdomain is removed
> > >
> > > Ah, I will test on a 32bit system, the code works for me on 64bits.
> >
> > You were right, one test (that checked gr_mem pointer for NULL for empty
> > groups) relied on undefined behaviour. I simply removed the code,
> > it was bogus, we are able to detect empty groups in the packet by the
> > number of members, which is also what the sss_client does.
>
> Thank you, now the unit test do not fail anymore. Nevertheless I think
> it would be good to remove this undefined behaviour and properly
> initialized the packet. If you agree would you mind to open a ticket?
>
> The patches look good so ACK. I only have a minor nitpick:
Sorry, I didn't express the problem clearly.
The packet *is* initialized correctly[1], just the code in the unit test
that parses the packet into struct group didn't initialize gr_mem to NULL
when it processed a packet that described an empty group didn't work the
same as in the client code.
If you prefer, I can amend the unit test parsing so that it behaves the
same as the client code..
grr, I forgot to add the footnote -- I actually verified that with a
simple C program. getgr* and getpw* result is correct.