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

Jakub Hrozek jhrozek at redhat.com
Mon Sep 14 18:20:46 UTC 2009


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 09/11/2009 10:56 PM, John Dennis wrote:
> 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.
> 

Thank you for the review, the comments should be fixed in patch attached
to Simo's review of the other two patches.

> * 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.
> 

Thanks, fixed

> * 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).
> 

Fixed.

> * 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.
> 

I know it's not ideal, but AFAIK there is no way to use tevent/confdb et
al. without talloc.

> * 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.
> 

Thanks, it appears that the macro is underdocumented on docs.python.org,
so I had no idea it even existed.

> * 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.
> 

Yes, I'm used to epydoc, so I actually forgot we should be using RST. I
changed the comments to RST, but it was actually my first time using it,
so maybe I didn't use some tags I should be or vice versa. Tested
rendering the docs with "epydoc --docformat=restructuredtext
pysss.local", looks sane.

	Jakub
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAkquiX4ACgkQHsardTLnvCX62wCdEI+8hm7x0ElI2oL/HKWHhRNe
By8AoMKtPGQJR68JRu9cDUbA9wpt2HjX
=UILg
-----END PGP SIGNATURE-----



More information about the sssd-devel mailing list