[SSSD] [PATCH] Add python bindings for murmurhash3

Jakub Hrozek jhrozek at redhat.com
Wed Aug 15 13:47:30 UTC 2012


On Wed, Aug 15, 2012 at 02:36:49PM +0200, Sumit Bose wrote:
> On Mon, Aug 13, 2012 at 11:28:07AM +0200, Jakub Hrozek wrote:
> > On Thu, Aug 09, 2012 at 02:20:23PM +0200, Sumit Bose wrote:
> > > Hi,
> > > 
> > > I would like to find a range/slice for Posix IDs based on a domain SID
> > > on the IPA server the same way as sssd does. For this I need python
> > > bindings for the murmurhash3() call. The attached patch adds them and a
> > > small test.
> > > 
> > > bye,
> > > Sumit
> > 
> > The code works as expected as is mostly OK, I would only like to see two
> > nitpicks fixed to make the code "more pythonic", see inline:
> > 
> > > +static PyObject * py_murmurhash3(PyObject *module, PyObject *args)
> > > +{
> > > +    const char *key;
> > > +    long key_len;
> > > +    unsigned long seed;
> > > +    uint32_t hash;
> > > +
> > > +    if (!PyArg_ParseTuple(args, "slk", &key, &key_len, &seed)) {
> > > +        return NULL;
> > > +    }
> > > +
> > > +    if (seed > UINT32_MAX || key_len > INT_MAX || key_len < INT_MIN ||
> > > +        key_len > strlen(key) || seed > UINT32_MAX) {
> > 
> > I think we should first set an exception here before returning NULL,
> > something like:
> > 
> >     PyErr_Format(PyExc_ValueError, "Invalid value\n");
> > 
> > > +        return NULL;
> > > +    }
> > > +
> > > +    hash = murmurhash3(key, key_len, seed);
> > > +
> > > +    return PyLong_FromUnsignedLong((unsigned long) hash);
> > > +}
> > 
> > > +class PySssMurmurImport(unittest.TestCase):
> > > +    def setUp(self):
> > > +        " Make sure we load the in-tree module "
> > > +        self.system_path = sys.path[:]
> > > +        sys.path = [ MODPATH ]
> > > +
> > > +    def tearDown(self):
> > > +        " Restore the system path "
> > > +        sys.path = self.system_path
> > > +
> > > +    def testImport(self):
> > > +        " Import the module and assert it comes from tree "
> > > +        try:
> > > +            import pysss_murmur
> > > +        except ImportError, e:
> > > +            print >>sys.stderr, "Could not load the pysss_murmur module. Please check if it is compiled"
> > > +            raise e
> > > +        self.assertEqual(pysss_murmur.__file__, MODPATH + "/pysss_murmur.so")
> > > +
> > > +class PySssMurmurTest(unittest.TestCase):
> > > +    def testExpectedHash(self):
> > > +        hash = pysss_murmur.murmurhash3("S-1-5-21-2153326666-2176343378-3404031434", 41, 0xdeadbeef)
> > > +        assert hash == 93103853
> > 
> > I think that using self.assertEqual(hash, 93103853) is more expected
> > here.
> > 
> > > +
> > > +if __name__ == "__main__":
> > > +    error = 0
> > > +
> > > +    suite = unittest.TestLoader().loadTestsFromTestCase(PySssMurmurImport)
> > > +    res = unittest.TextTestRunner().run(suite)
> > > +    if not res.wasSuccessful():
> > > +        error |= 0x1
> > > +        # need to bail out here because pyhbac could not be imported
> > 
> >                                            ^^^ copy-n-paste bug :-)
> > > +        sys.exit(error)
> > > +
> > > +    # import the pyhbac module into the global namespace, but make sure it's
> > 
> >                     ^^^ also here
> > 
> > > +    # the one in tree
> > > +    sys.path.insert(0, MODPATH)
> > > +    import pysss_murmur
> > > +
> > > +    suite = unittest.TestLoader().loadTestsFromTestCase(PySssMurmurTest)
> > > +    res = unittest.TextTestRunner().run(suite)
> > > +    if not res.wasSuccessful():
> > > +        error |= 0x2
> > > +
> > > +    sys.exit(error)
> > 
> > The rest looks good to me.
> 
> Thank you. The attached new version fixes the issues you mentioned,
> improves the parameter validation and adds tests for the new exceptions.
> 
> bye,
> Sumit

Ack



More information about the sssd-devel mailing list