[SSSD] [PATCH] sss_cache: Remove fastcache even if sssd is not running.

Michal Židek mzidek at redhat.com
Thu Nov 1 10:19:00 UTC 2012


On 11/01/2012 06:54 AM, Simo Sorce wrote:
> On Wed, 2012-10-31 at 12:43 +0100, Michal Židek wrote:
>> On 10/30/2012 02:25 PM, Simo Sorce wrote:
>>> On Tue, 2012-10-30 at 10:39 +0100, Michal Židek wrote:
>>>> On 10/29/2012 03:53 PM, Simo Sorce wrote:
>>>>>
>>>>> Codewise looks ok, but I still see duplication of the code used to lock
>>>>> the file.
>>>>>
>>>>> I was wondering, would it make sense to split this patch in 2 and put
>>>>> the lock_mc_file functions as a more generic function in the common
>>>>> utils as a separate patch first ?
>>>>>
>>>>> I would see it added in util/util_lock.c so it is available to both
>>>>> tools and responders (avoids the duplication you still have).
>>>>>
>>>>> The prototype should be something like:
>>>>>
>>>>> static errno_t sss_br_lock_file(int fd, size_t start, size_t len, int
>>>>> retries, int wait);
>>>>>
>>>>> Where wait is expressed in milliseconds.
>>>>>
>>>>
>>>> I was thinking about this too, but there is one thing I do not like
>>>> about it. Maybe I am wrong, but IMO functions related to memory cache
>>>> should be as private to sssd_nss as possible. That is why the function
>>>> for locking the files is static (sssd_nss part). Function for memory
>>>> cache is invalidation is an exception to this rule, because it is used
>>>> by sss_cache, but it should also keep the code that manipulates memory
>>>> cache private (and it uses it only on one place, so static function was
>>>> not needed that much).
>>>
>>> This is why I made it a generic function not strictly related to the
>>> cache in the prototype.
>>> I prefer to keep a single function to be used by all components, because
>>> copy&paste is a source of pain in the long term.
>>>
>>>> I do not know about any other place, where we would like to lock the mc
>>>> files except of sssd_nss and the sss_memcache_invalidate used by
>>>> sss_cache and it should stay like this. I consider the usage of mc files
>>>> in sss_cache as necessary pollution, but providing non-static API to
>>>> spread this pollution does not sound good to me.
>>>
>>> The function prototype I suggested is not mc specific. Look at it, it
>>> can be used with any file to lock any part, we can also extend it to
>>> take a lock type if you want it even more generic.
>>>
>>>> But if you really want the util_lock.c, I can send it as a separate
>>>> patch on top of this one (it is only a small refactor). But still... I
>>>> am not sure if this is good idea.
>>>
>>> I think it should be a patch that comes before the current one and just
>>> provides the facility, then the current patch will build on top of it.
>>>
>>
>> Ok. I did not add the parameter to select the type of lock (not all
>> types would fit to this function). But we can expand the util_lock.c
>> later with additional functions if more lock types are needed.
>>
>>>>> Also you do not really need to unlock the file if you are going to
>>>>> close() it.
>>>>> Posix semantics require the OS to drop all locks on a file if you close
>>>>> it, so you can safely drop the logic around unlocking in tools_util.c
>>>>> (however add a comment before the close like: /* closing the file will
>>>>> drop the lock */ ).
>>>>
>>>> I thought closing the file explicitly is more readable. But yes, I'll
>>>> remove it, adding the comment as you suggested is better.
>>>
>>> Yup, thanks, this part looks good, I think we only need to split the
>>> patch and move the locking function into util, then we are golden.
>>>
>>> Simo.
>>>
>>
>> New patches attached.
>
> Michal,
> the structure now is fine, only one minor nitpick is that it would be
> probably better to remove references to mmap cache from sss_br_lock_file
> debugging messages as it is not mmap cache specific anymore.

Sure, this was leftover from previous versions.

>
> Anyway I was trying to test your patch on my system but on current
> master, with your 3 patches on top I cannot get sss_cache to do anything
> as far as I can see.
>
> It always returns: "No cache object matched the specified search"
>
> Looking at the code it seem to me that this always happen (skipped is
> always true if you pass only one switch ?) unless all maps are full and
> you pass -UGNSA, but I haven't used this tool much so I may just be
> missing something.
> What's the right way to test this patches ?
>

This is strange. It works fine for me. I test it for example this way:

1. start sssd
2. fill the cache with getent passwd user1
3. turn off sssd
4. getent passwd user1 again (result is returned from memory cache)
5. sss_cache -u user1 (or sss_cache -U) should remove the cache
6. getent passwd user1 should give no results

I also tested the sss_cache while sssd was running and it gives me 
expected results (sended SIGHUP to monitor and next getent did not use 
the mem cache).

I tested it on current master, but maybe there really is a bug. If it is 
still not working for you, could you send how you test it?

Thanks
Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-util-Added-new-file-util_lock.c.patch
Type: text/x-patch
Size: 4190 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121101/bb59e4cb/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sss_cache-Remove-fastcache-even-if-sssd-is-not-runni.patch
Type: text/x-patch
Size: 9515 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121101/bb59e4cb/attachment-0001.bin>


More information about the sssd-devel mailing list