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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
