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