[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