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

Simo Sorce simo at redhat.com
Tue Oct 30 13:25:27 UTC 2012


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.

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

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




More information about the sssd-devel mailing list