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:
> 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
> 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.
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).
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..