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

[John Ferlan]

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.

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.

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.

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


John