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.patchPushed 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
