[libvirt] [libvirt-python PATCH 00/23] Cleanup of the libvirt-python C code

[Pavel Hrdina]

On Sat, Sep 26, 2015 at 10:52:27AM -0400, John Ferlan wrote:
> 
> 
> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
> > This patch series tries to cleanup the libvirt-python C code and also make it
> > more readable.  It also fixes places, where we didn't checked for return values
> > and where we also returned wrong values in case of errors.  There are some other
> > minor issues, that I've found.
> > 
> > Pavel Hrdina (23):
> >   update virDomainGetVcpus xml API description
> >   refactor the function to not override python exceptions
> >   remove useless check for NULL before Py_XDECREF
> >   drop unnecessary goto
> >   Move utils and shared code into libvirt-utils
> >   cleanup functions definition
> >   indent labels by one space
> >   fix indentation
> >   wrap lines to 80 columns
> >   Return NULL if python exception is set
> >   Return correct python object
> >   Return NULL and set an exception if allocation fails
> >   Use VIR_PY_NONE instead
> >   use Py_CLEAR instead of Py_XDECREF followed by NULL assignment
> >   all Py*_New function has to be checked for return value
> >   change the order of some statements
> >   drop unnecessary py_retval variable
> >   improve usage of cleanup paths
> >   utils: introduce new macro helpers for tuple, list and dict objects
> >   use VIR_PY_TUPLE_GOTO
> >   use VYR_PY_LIST_SET_GOTO and VIR_PY_LIST_APPEND_GOTO
> >   use VIR_PY_DICT_SET_GOTO
> >   coverity: resolve dead_error_condition
> > 
> >  libvirt-lxc-override.c   |   61 +-
> >  libvirt-override-api.xml |    4 +-
> >  libvirt-override.c       | 3156 +++++++++++++++++++++-------------------------
> >  libvirt-qemu-override.c  |   60 +-
> >  libvirt-utils.c          |  422 ++++++-
> >  libvirt-utils.h          |  133 ++
> >  typewrappers.c           |   65 +-
> >  7 files changed, 2047 insertions(+), 1854 deletions(-)
> > 
> 
> *Lots* of monotonous changes... I have to assume for those places where
> you've changed return values and "followed the rules" regarding what
> gets returned when, you ended up going through all functions (a tedious
> task). Obviously I couldn't do the same, so I do trust you've followed
> those unwritten rules (or are they written somewhere?  Maybe updating
> "HACKING" would help; otherwise, you ended up having to be very vigilant
> with every change.

Actually the rule is written somewere in the cpython documentation.  It would be
probably worth mentioning it in HACKING.

> 
> If I commented on something - it would need adjustment before push
> 
> If I didn't comment on something, then an implicit ACK may be assumed.

I'll fix the nits you've found and probably send few v2 patches to be reviewed
again.

> 
> Would have been a lot easier to have 3 groups of 8 (or so) rather than
> one group of 23 ;-)... Especially those long patches, but I understand
> why it was done at all one time.

It would have been a lot easier, but I started with just a simple cleanup and
ended up whit those 23 patches :)

> 
> Hopefully it's a lot easier to maintain - I do think it would be wise to
> create some 'rules' or 'guidelines' in HACKING (assuming anyone reads
> it).  If they don't and mess up, you can always point them at the
> document during review.
> 
> Lots and lots of error path cleanup, which is great! It'll be
> interesting if any 'consumer' up our short stack starts tripping on
> these changes.  Might be good to give perhaps a strong heads up to allow
> time for them to "ferret out" any issues...

I've tried to run virt-manager tests on top of those changes in libvirt-python
and there was no issue at all, so we will see :)

Thanks for the review,

Pavel

> 
> 
> John