On Mon, Nov 05, 2012 at 01:26:04PM +0100, Michal Židek wrote: > 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: > >>> > >>>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. > > > > 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 >
Pushed to master and sssd-1-9 _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
