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
Jan