On Tue, Apr 02, 2013 at 02:47:47PM +0200, Jakub Hrozek wrote:
On Fri, Mar 29, 2013 at 02:47:46PM +0100, Pavel Březina wrote:
> On 03/27/2013 01:44 PM, Jakub Hrozek wrote:
> >On Wed, Mar 27, 2013 at 11:03:46AM +0100, Lukas Slebodnik wrote:
> >>On (25/03/13 14:54), Jakub Hrozek wrote:
> >>>On Mon, Mar 25, 2013 at 02:48:44PM +0100, Pavel Březina wrote:
> >>>>On 03/22/2013 10:59 PM, Lukas Slebodnik wrote:
> >>>>>On (21/03/13 16:10), Pavel Březina wrote:
> >>>>>>On 03/21/2013 03:43 PM, Lukas Slebodnik wrote:
> >>>>>>>On (22/02/13 13:46), Pavel Brezina wrote:
> >>>>>>>>----- Original Message -----
> >>>>>>>>>From: "Pavel Březina"
<pbrezina(a)redhat.com>
> >>>>>>>>>To: sssd-devel(a)lists.fedorahosted.org
> >>>>>>>>>Sent: Thursday, February 21, 2013 11:43:26 AM
> >>>>>>>>>Subject: [SSSD] [PATCH] refactor nested group
processing
> >>>>>>>>>
> >>>>>>>>>https://fedorahosted.org/sssd/ticket/1784
> >>>>>>>>
> >>>>>>>>I should have probably add that this is the final
version. I will attach unit test once I get familiar with cmocka, but it does not really
prevent reviewing the patch.
> >>>>>>>
> >>>>>>>I would like to test your patches, but the second patch
no longer applies
> >>>>>>>on the current master. Could you resend rebased
patches?
> >>>>>>
> >>>>>>Thanks. Rebased patches are attached. I did not test it
again though,
> >>>>>>but it compiles fine and the diff looks all right. It may
be
> >>>>>>interesting to determine what patch broke it.
> >>>>>>
> >>>>>
> >>>>>I had problems with you patches.
> >>>>>The simplest cycle schema:
> >>>>> ________
> >>>>> | CG1 |------------>sssduser1
> >>>>> |______|<---|
> >>>>> | |
> >>>>> | |
> >>>>> | ________
> >>>>> |---->| CG2 |--->sssduser2
> >>>>> |______|
> >>>>>
> >>>>>sssd client configured with ipa-client-install and two
additional options
> >>>>>entry_cache_timeout = 5
> >>>>>memcache_timeout = 1
> >>>>>
> >>>>>How to reproduce.
> >>>>>1. remove cache(also logs, but it is not important)
> >>>>>2. service sssd start
> >>>>>3. run commands
> >>>>>
> >>>>>sh-4.2$ getent group cyclegroup1
> >>>>>cyclegroup1:*:30007:sssduser1,sssduser2
> >>>>>sh-4.2$ getent group cyclegroup2
> >>>>>cyclegroup2:*:30008:sssduser2
> >>>>> #after second (memcache_timeout)
> >>>>>sh-4.2$ getent group cyclegroup2
> >>>>>cyclegroup2:*:30008:sssduser2,sssduser1
> >>>>>
> >>>>>-----------------------------
> >>>>>
> >>>>>Also sometimes with the same groups scheme one user appeared two
times,
> >>>>>but I could not exactly reproduce steps.
> >>>>
> >>>>The problem is even in master. Seems to be another ghost users
issue.
> >>>>
> >>>
> >>>I agree, the EEXIST error is coming from memberof plugin:
> >>>https://fedorahosted.org/sssd/ticket/1846
> >>>
> >>>>>
> >>>>>If you could not reproduce problem, I could send you off the
list
> >>>>>logs, cache, any configurations, rpms or even provide access to
IPA server.
> >>>>>
> >>>>>And last notice, commit
4709ff46db0dbe073aef061b796d2fd7adeaf18f
> >>>>>with title "LDAP: If deref search fails, try again without
deref"
> >>>>>cause little problem with applying patches.
> >>>>>
> >>>>>LS
> >>>>
> >>>>I amended my patches to keep the functionality introduced in this
> >>>>commit. I.e. if deref fails with ENOTSUP, we will try again without
> >>>>dereference.
> >>>>
> >>
> >>I tested functionality of your patches and except bug from ticket
> >>(https://fedorahosted.org/sssd/ticket/1846) everything work like expected.
> >>Tested cases:
> >> --nonexisting group
> >> --empty group
> >> --user in group
> >> --nested groups
> >> --user in more nested group(no duplication)
> >> --cycle
> >> --deref
> >> --nonderef
> >>
> >>I have to say, that I learnd a lot of new things.
> >>
> >>(mini Ack) -> So someone else could look to code.
> >>I am not yet expert in tevent coding style
> >>
> >>LS
> >
> >Thanks for testing!
> >
> >I reviewed the code today and I only have some suggestions given that
> >Lukas' testing went fine.
> >
> >In general when debugging the nested group code in the past, I missed
> >more DEBUG information about the progress (as opposed to just errors).
> >This is getting better in the attached patch but maybe more could still
> >be added to the utility functions at the start of the module or
> >sdap_nested_group_recv() for example.
>
> Done.
>
> >There could be a simple wrapper macro around
> >sdap_nested_group_search_cache named _sysdb_search_{users,groups}, that
> >would be more readable than true|false, similar to how you coded up
> >sdap_nested_group_hash_{user,group}.
>
> Done.
>
> >step_down is a misleading variable name, recurse would be much better.
> >
> >In the sdap_nested_group_single_ request, instead of reusing the _done
> >callback for both direct process and nesting process, can you create
> >another callback? I think that would be more readable. The same about
> >sdap_nested_group_deref_done. I don't think the flow should be
> >controlled by boolean variables that much.
>
> Done.
>
> >You're right that sdap_nested_group_get_ipa_user() should be set as a
> >function pointer from IPA backend, but it's unlikely to happen in 1.10.
> >Please file a ticket about that.
> >
> >The lookup_uknown request could be split into better steps, too. Instead
> >of relying on state->type, why not have a callback after check for user,
> >then check for groups go to another callback and if either of them
> >succeeds, finish the request. I know there was a similar flow in the
> >previous version, but there the steps were shared inside one
> >super-request. With smallish requests in this version, I think it would
> >be nicer than stricly keeping one callback.
>
> Done.
>
> I like it more how it was before though. We called tevent_req_done()
> only from one place which was nice and state->type has to be set
> correctly anyway.
>
Yes, but the current flow is more readable, you can follow the code of
the request from top to bottom and it's always clear what happens next.
The code now looks good to me and passed my tests (which were by no
means as complete as Lukas' testing). I'm still not quite sure about the
flow of the sdap_nested_group_single_* request but it's a big
improvement over the previous state.
We can do more refactoring later if we see fit, right now I don't want
to hold off the patch any longer.
Ack.
Pushed to master.