[SSSD] [PATCHES] python bindings for managing users in local domains

John Dennis jdennis at redhat.com
Fri Sep 11 20:56:19 UTC 2009


I've reviewed the python binding for sss. It looks pretty good Jakub. I 
only have a few comments and most of these are style issues as opposed 
to a coding or logic flaw.

* In PyErr_SetSssErrorWithMessage() you don't need to call the 
PyExc_IOError constructor, instead use Py_BuildValue and use the 
returned tuple as the exception value. To be honest I'm not sure what it 
means to set the value of an exception to another exception which is 
what it appears to be what you're doing.

* Some of the function parameters are python objects obtained using the 
"O" format specifier for PyArg_ParseTuple. However in each instance they 
can't be any python object, rather they must be List objects. You do 
check the object type when you pass the object to PyList_AsStringList, 
but I'd rather see the validation of the object type happen earlier and 
follow existing Python convention of throwing a type exception directly 
from PyArg_ParseTuple. This can easily be done by specifying the format 
specifier as "O!" rather than "O". When you do this you must add an 
additional argument to PyArg_ParseTuple which is a pointer to the object 
type. For example

PyArg_ParseTuple(args, "O!", &PyList_Type, &py_list)

This way the error is caught before any processing is done in the 
function and it then become explicitly obvious to someone reading the 
code what type of object the parameter must be for correct operation. It 
also means the error message will exactly match all other parameter type 
errors thrown by python (consistency has a value).

* You're using two different memory management libraries, talloc and 
Python's. It does appear you're keeping the usage of the two separate 
which is critical (however I didn't carefully check all the usage). 
However mixing two different memory management libraries in one body of 
code makes me nervous, the opportunity to call the wrong library 
function on a pointer with disastrous consequences is always lurking. I 
presume you're using talloc because sss needs it, but if you can avoid 
mixing two different memory management libraries in one body of code 
you'll eliminate one potential source of obscure bugs.

* Documentation strings. I'd rather see the docmentation strings appear 
next to the functions they are documenting, not in a far off place in 
the file. That way when someone is reading the code they can see the 
documentation for the function they're reading and more importantly they 
can see the parameter lists and correlate that with argument extraction 
(PyParseTuple). PyDoc_STRVAR() is a common convention.

* Documentation format. In IPA we've decided to use reStructured text as 
the markup (see 
http://freeipa.com/page/Python_Coding_Style#Python_API_documentation). 
Your docstrings look like they're in epydoc markup.

Overall the code looks good, you've done a good job.

-- 
John Dennis <jdennis at redhat.com>

Looking to carve out IT costs?
www.redhat.com/carveoutcosts/



More information about the sssd-devel mailing list