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