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

Simo Sorce simo at redhat.com
Thu Nov 1 13:45:22 UTC 2012


On Thu, 2012-11-01 at 09:14 -0400, Simo Sorce wrote:
> On Thu, 2012-11-01 at 11:19 +0100, Michal Židek wrote:
> > 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?
> 
> I tired the same above commands, however I didn't make sure to
> repopulate the cache after the first try, maybe I was indeed operating
> on an empty one, I'll retry with your 2 new patches (btw it would be
> nice if you could send them in separate emails in future, so patchwork
> picks both them up :).


Ok I have found out what I was doing wrong.
I was trying with a user in a trusted domain. These users are apparently
not yet stored in the memory ccache (need to open a bug on that I
guess).

Once I started trying with an simple ipa users it worked fine.

ACK!

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list