On Mon, 2012-03-19 at 14:40 +0100, Jan Zelený wrote: > > On Mon, 2012-03-19 at 10:21 +0100, Jan Zelený wrote: > > > > > responder/nss/nsssrv_mmap_cache.c:220 > > > > > - this condition is unnecessarry because of the condition on line 208 > > > > > > > > Can you post the actual code snippest, if I understand what you mean I > > > > think I have line numbers off by 1 ... but also do not see the > > > > unnecessary condition. cur is adavanced since the test in line 208/209 > > > > so it is not testing the same thing afaics. > > > > > > I hope I read the code correctly, this is by far the most complex part of > > > all your patches. > > > > > > First you have this condition: > > > > > > if (mcc->free_table[t] == 0xff) { > > > > > > cur = ((cur + 8) & ~7); > > > if (cur >= tot_slots) { > > > > > > cur = 0; > > > > > > } > > > continue; > > > > > > } > > > > > > > > > Right after that you have following cycle with condition right after it: > > > for (t = ((cur + 8) & ~7) ; cur < t; cur++) { > > > > > > MC_PROBE_BIT(mcc->free_table, cur, used); > > > if (!used) break; > > > > > > } > > > if (cur == t) { > > > > > > if (cur >= tot_slots) { > > > > > > cur = 0; > > > > > > } > > > continue; > > > > > > } > > > > > > Considering the condition if (mcc->free_table[t] == 0xff), the cur will > > > never equal to t. That would imply that there was no bit set to zero in > > > mcc- > > > > > > >free_table[t], which is already ruled out by the previous condition. > > > >Instead > > > > > > of this condition, I'd simply place a comment above the for cycle: "There > > > is a zero bit in the byte, find it". I think it would improve > > > readability of the code. > > > > Ah excellent analysis, you are totally correct. > > I attached a patch that removes the unnecessary check and adds comments > > to make it easier to read. > > > > > > > responder/nss/nsssrv_mmap_cache.c:242 > > > > > - the assignment is redundant > > > > > > > > I guess it's t, yeah that's a leftover .. > > > > > > > > > responder/nss/nsssrv_mmap_cache.c:275 > > > > > - why this condition? I think the following while will cover it > > > > > > > > slot is used to dereference data from the table, if it accesses beyond > > > > the table it will cause a segfault. > > > > > > I know that, my point was that from my understanding the slot variable > > > can either contain valid number or MC_INVALID_VAL, nothing else. Of > > > course if I missed anything or you are just being defensive just in > > > case, I withdraw my comment. > > > > Purely defensive yeah. > > > > Simo. > > Much more readable now, thank you > > Ack to all patches. When pushing, please note that this one replaces the > original 0003-nsssrv-Add-memory-cache-record-handling-utils.patch
Pushed to master. Exciting!
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