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!