> On Thu, 2012-01-12 at 11:49 -0500, Simo Sorce wrote: > > On Tue, 2012-01-10 at 14:33 -0500, Simo Sorce wrote: > > > On Tue, 2012-01-10 at 10:15 -0500, Simo Sorce wrote: > > > > > Sure, we can talk about it. I'm looking at it from the users' > > > > > perspectives, who I think would generally expect (and be alright > > > > > > > > with) > > > > > > > > > the fast cache being emptied on service restart. Since we still > > > > > have > > > > > > > > the > > > > > > > > > not-quite-as-fast persistent LDB cache, I think the gain isn't > > > > > worth > > > > > > > > the > > > > > > > > > user confusion. > > > > > > > > Ok, I think I can understand this, it will also simplify the code I > > > > guess so I'll change the patch. > > > > > > Attached new set of patches. > > > > > > I changed the init code to always create a new cache file on restart, > > > incidentally this also got rid of the stat() call, so that problem has > > > been put to rest permanently :) > > > > > > I also changed all functions to use errno_t as the return type in their > > > declaration when they return a errno code. > > > > > > I think all the suggestions seen in the thread so far have been > > > implemented as requested at this point, with the exception of the part > > > where I should implement and use new options as that required > > > additional code instead of mere refactoring. I will add those features > > > in a later rebase. > > > > New revision. > > > > I removed bootid from the cache file header, given we recreate the file > > every time sssd restarts it was useles. > > > > I replaced it with a seed, and made the seed used in the hash table > > randomly regenerated every time the hash table is recreated. > > > > This is used to avoid collision based attacks [1]. > > > > Simo. > > > > [1] https://lwn.net/Articles/474912/ > > I started looking at these last night. I have yet to complete a full > review, but I'm attaching a set of patches rebased atop the current > master so several of us can start reviewing.
Ok, I finished the review. I am fine with these patches in general, I have just a few comments and/or questions: Why only one hash table for both name and uidkey? I think using two would make things a little bit more simple and fast (of course a tradeoff would be slightly higher memory consumption). responder/nss/nsssrv_mmap_cache.c:220 - this condition is unnecessarry because of the condition on line 208 responder/nss/nsssrv_mmap_cache.c:242 - the assignment is redundant responder/nss/nsssrv_mmap_cache.c:275 - why this condition? I think the following while will cover it responder/nss/nsssrv_mmap_cache.c:516 - sizeof(uint32_t) -> sizeof(h.status) responder/nss/nsssrv_mmap_cache.c:517 - sizeof(uint32_t) -> sizeof(h.status) responder/nss/nsssrv_mmap_cache.c:561 - shouldn't the umask better be 0133? close vs. munmap Just out of curiosity: does it matter which should be first? I found two places in the code where the order is different: sss_client/nss_mc_common.c:146 responder/nss/nsssrv_mmap_cache.c:717 That's all from my side. As I've said, my comments are rather minor things and suggestions, so in general I give these patches a green light. Thanks Jan
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel