[SSSD] [PATCH 0/3]: New utility functions/headers

Stephen Gallagher sgallagh at redhat.com
Mon Jan 9 18:34:52 UTC 2012


On Mon, 2012-01-09 at 13:27 -0500, Simo Sorce wrote:
> On Mon, 2012-01-09 at 12:58 -0500, Stephen Gallagher wrote:
> > On Tue, 2012-01-03 at 18:00 -0500, Simo Sorce wrote:
> > > >From [PATCH 0/0] A shared memory cache to perform better:
> > > 
> > > 0/3: New utility functions/headers
> > > These are also quite straightforward but they are useless without the
> > > later patches
> > 
> > Nack.
> > 
> > Your implementation doesn't match the original:
> > 
> > 
> > From MurmurHash3_x86_32():
> >     h1 = h1*5+0xe6546b64;
> > 
> > From this patch:
> >         h1 = h1 + 5 + 0xe6546b64;
> > 
> > I can only assume that s/*/+ is not going to yield proper results :)
> 
> OUPS, well it will give *different* results, but consistent.
> Anyway I'll fix it and send back a fixed patch.
> 
> > 
> > I fail to understand why you use (nblocks * 4) instead of 'len', given
> > that nblocks was defined as len/4
> 
> Because (nblocks * 4) != len and it is 3 times out 4 < len :-)
> Remember nblocks is an integer, so any fractional part is lost, which is
> what we want here, we want to process len - (nblocks * 4) remaining
> bytes.
> 

Thanks, I was misreading it.

> > While I know that it's a fixed value, I'd rather see sizeof(uint32_t)
> > instead of '4' in getblock() (for readability and clarity)
> 
> Ok.
> 
> > As for the tests, I'd prefer them to guarantee also that the same string
> > hashes to the same value twice. Just in case it's not actually
> > deterministic :)
> 
> I don't see how it can't be, but I guess it could catch bugs where some
> uninitialized variable is used by mistake ?
> 

Yeah, it could catch uninitialized variables or memory corruption due to
buffer overrun, off-by-one error, etc.

> > The tests there aren't too exhaustive, given that it only guarantees
> > that six pre-chosen strings don't collide.
> 
> I used them to find a bug in the 1st implementation :-)
> I don't think we really need exhaustive tests here, upstream has a lot
> more tests as they are calculating the actual collision rates and
> comparing them to other hashes, but we don't need to. We can trust
> upstream got the algorithm good enough.
> 
> But if you have any important test to suggest I can try to add them.


That was just me noting that they were only marginally useful. I don't
have any great ideas otherwise. You're right about upstream being fine,
as long as we keep track of any changes they make.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120109/c7d0786b/attachment.sig>


More information about the sssd-devel mailing list