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