[SSSD] [PATCH] The new memberof plugin

Stephen Gallagher sgallagh at redhat.com
Thu Sep 22 10:05:44 UTC 2011


Purge or expire the cache
getent group groupthatexistsandhasmembers

I tested on an RFC2307 server.

On Sep 22, 2011, at 5:10 AM, Jan Zelený <jzeleny at redhat.com> wrote:

>> On Wed, 2011-09-21 at 13:24 +0200, Jan Zelený wrote:
>>>> On Tue, 2011-08-30 at 17:54 +0200, Jan Zelený wrote:
>>>>> Attached patch rewrites almost entire memberof plugin. It heavily
>>>>> utilizes hash tables instead of lists and arrays and it introduces
>>>>> concept of reference counting which should heavily optimize all
>>>>> operations when no loops are present in the user/group tree.
>>>>> 
>>>>> I tested the patch by running sysdb test suite, all tests passed. I
>>>>> also attach a document where basic concepts of the plugin are
>>>>> explained.
>>>>> 
>>>>> Please note that the patch is functional, although I don't consider
>>>>> it ready. I just want to get pre-ACK or some comments about the
>>>>> patch design. I have yet to implement the recompute task, I will
>>>>> work on that later.
>>>> 
>>>> Jan,
>>>> yesterday I an Stephen got together to start a review of the patch. Due
>>>> to the complexity we tried to go thorugh the code together to make sure
>>>> we understand what is going on.
>>>> 
>>>> Also due to the complexity and need to refresh memory we were only be
>>>> able to go through most of the "add" code only, we were not able to
>>>> review the update or delete code paths yet.
>>>> 
>>>> Attached find a diff file with comments in C++ style (so that it is
>>>> clear they all need to be resolved before the patch can be considered
>>>> ok, as they show as a big red line in our editors) that you can
>>>> integrate in your tree and check one by one.
>>>> 
>>>> There are some questions in there, but in general we noticed a very
>>>> annoying lack of comments in the code itself. The accompaning PDF is
>>>> nice and all, but unless the logic is embedded and explained in the
>>>> code (like it was with the previous code) it will be ablocker and will
>>>> make review more difficult.
>>>> 
>>>> That said, so far so good, although we need to more carefully review
>>>> the refcounting stuff, we got to it late and only marginally reviewed
>>>> it. Need more time to fully grasp all details.
>>>> 
>>>> HTH,
>>>> Simo.
>>> 
>>> Simo,
>>> as promised before, I have a new version of the patch. This time it's
>>> complete, including the rewritten recompute task. I also took almost all
>>> comments you and Stephen gave me into account. The only thing I left
>>> unresolved for the moment is the last comment in the diff you sent. The
>>> change shouldn't be difficult, but it will need changes in many parts of
>>> the code, therefore I'll rather send it in the next patch.
>> 
>> Nack
>> 
>> First, the third patch in this set does not compile. You used
>> sysdb_memberof_recompute() in the test, but the function is
>> sysdb_memberof_rebuild().
>> 
>> With that typo fixed, I attempted to run with this patch. Any attempt to
>> run getgrnam() or getgrgid() results in a hang in the LDB until
>> eventually the monitor issues a SIGTERM to the backend (which doesn't
>> die, because it never gets back to the mainloop to process the signal).
> 
> 
> Could you please send me steps how to reproduce?
> 
> Thanks
> Jan


More information about the sssd-devel mailing list