[libvirt] [PATCHv2 0/6] remaining cleanups to libvirt.c

[John Ferlan]

On 01/14/2014 04:43 PM, Eric Blake wrote:
> v2 of my patch series started here, after applying all patches
> already reviewed in that thread:
> https://www.redhat.com/archives/libvir-list/2013-December/msg01284.html
> 
> Patches 1-3 can be considered bug fixes (particularly patch 3),
> so I'd like them in 1.2.1 if they get a favorable review.  Patches
> 4-6 are less urgent, but might as well finish the work all within
> one release.
> 
> Patch 1 comes from 5/24 in v1
> Patch 2 is new
> Patch 3 and 4 come from 23/24 in v1
> Patch 5 is new
> Patch 6 comes from 24/24 in v1
> 
> Eric Blake (6):
>   maint: don't leave garbage on early API exit
>   maint: avoid nested use of virConnect{Ref,Close}
>   maint: don't lose error on canceled migration
>   maint: clean up error reporting in migration
>   maint: simplify driver registration at startup
>   maint: replace remaining virLib*Error with better names
> 
>  cfg.mk                       |  10 --
>  src/libvirt.c                | 397 ++++++++++++++++++++-----------------------
>  src/lxc/lxc_process.c        |  11 +-
>  src/qemu/qemu_driver.c       |  12 +-
>  src/qemu/qemu_migration.c    |  14 +-
>  src/qemu/qemu_process.c      |  10 +-
>  src/storage/storage_driver.c |   5 +-
>  src/uml/uml_driver.c         |   3 +-
>  8 files changed, 209 insertions(+), 253 deletions(-)
> 


Patch 3 seems to be the one where there would be a couple of migration
fixes that could use a backport, especially the change in
virDomainMigrateVersion3Full() where the code now actually honors the
condition where the uri was not set rather than just playing dumb. I
also keep looking the changed "single" (!ret) to a "multi-state" (!ret
&& !xxx) condition and keep asking myself could we run into a situation
where (!ret and xxx) is true where previously we'd fail on the !ret, but
now wouldn't because xxx was true (where xxx is retcode or cancelled).

Another "step" to consider for patch 5 would be to make common the
repetitive code that follows the virDriverCheckTabMaxReturn(), e.g.

    VIR_DEBUG('string')
    table[count] = driver
    return count++

I don't see a reason other than proximity to release date to preclude
inclusion in 1.2.1

ACK series in general though.

John