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