On 11/01/2012 02:45 PM, Simo Sorce wrote:
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:
- start sssd
- fill the cache with getent passwd user1
- turn off sssd
- getent passwd user1 again (result is returned from memory cache)
- sss_cache -u user1 (or sss_cache -U) should remove the cache
- 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.
Just re-sending these patches. They are the same as previous just rebased so that they apply on top of the modified 'sss_cache: Multiple domains not handled properly' patch.
Thanks Michal