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

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