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
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel