[SSSD] [PATCH 0/4]: Actual memory cache implementation

Jan Zelený jzeleny at redhat.com
Mon Mar 19 13:40:38 UTC 2012


> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120319/c5c9040b/attachment.sig>


More information about the sssd-devel mailing list