10:41 a.m.
On Wed, Dec 05, 2012 at 08:30:01AM -0500, Simo Sorce wrote:
On Wed, 2012-12-05 at 10:18 +0100, Jakub Hrozek wrote:
> On Wed, Dec 05, 2012 at 12:39:33AM -0500, Simo Sorce wrote:
> > On Tue, 2012-12-04 at 21:39 +0100, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > this thread obsoletes the previous memberof related threads into a single
one
> > > to make it easier for the reviewers to apply the patches without having
to
> > > guess the dependencies. The patches should apply cleanly on origin/master.
In
> > > addition to bringing all the patches together, this set also addresses
some
> > > of Simo's concerns, in particular the ghost users are better explained
in
> > > the big comment blocks above each major memberof operation.
> > >
> > > The patches implement the missing operations of the memberof plugin
> > > related to ghost users - delete and modify. The mod operation is given
> > > some special care because not only we need to propagate the modification
> > > to parent groups, but we also need to retain the inherited ghost
attributes.
> > >
> > > The intent is to fix issues such as #1652. Most patches also include
> > > unit tests to make sure we don't break existing memberof plugin
> > > functionality.
> > >
> > > More information is in the patches themselves or in the commit messages.
> >
> > The first 5 patches get a tentative ack from me.
> > They look good and they compile fine and a basic smoke test seem to show
> > no issue whatsoever, also make check works as expected.
> >
> > On the last patch I have a question. I see that you do a number of base
> > searches for each hild of a group in order to source ghost users. But
> > that seem a bit wasteful.
> >
> > Why don't you simply do a subtree search on the whole tree with
> > (&(objectclass=group)(memberof=<dn of the group you are handling>))
(you
> > might also add a &(ghost=*) ) ?
> >
> > This would give you all groups that are 'children' of the group you
are
> > actually handling with a single search and avoids searching user entries
> > and then immediately discarding them as they have no ghost members.
> >
> > Simo.
>
> Mostly because memberof is transitive so I thought I might get too
> many "ghost" duplicates in case the nesting went too deep which would
> need to be filtered later (using the muop operations).
Sorry not sure I understand what you mean here, the number of users in
the typical case is much higher than groups, I wouldn't worry in
processing groups that add only duplicates.
> On the other hand, in the typical case, there would probably *many* more
> user entries to discard when traversing direct members rather than
> memberofs, so you might be right..
IT really depends on how many users are actually fetched, if you have
many ghost you'll probably have few users, but then if you have few
ghost it may be because a lot of users were actually resolved.
On the filtering of ghosts I was wondering if it wouldn't be faster to
sort ghosts by name in the structure where you gather them. It would
make searching for a match faster if you then use a bisection algorithm,
or use a simple hash table :)
This optimization doesn't need to be done right now, but maybe we open a
ticket for future improvement.
As it turned out, it is really needed to filter out duplicated at the
time we gather them. The attached new iteration of patches uses hash
table to accomplish this.