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