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

Michal Židek mzidek at redhat.com
Tue Oct 30 09:39:25 UTC 2012


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

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.

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.

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

>
> Simo.
>

New patch is attached.

Thanks
Michal

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-sss_cache-Remove-fastcache-even-if-sssd-is-not-runni.patch
Type: text/x-patch
Size: 11645 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20121030/3bce303d/attachment.bin>


More information about the sssd-devel mailing list