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.
--
Simo Sorce * Red Hat, Inc * New York