Re: [libvirt] [PATCH 23/24] maint: clean up error reporting in migration

On 12/28/2013 11:11 AM, Eric Blake wrote: > While auditing the error reporting, I noticed that migration > had some issues. Some of the static helper functions tried > to call virDispatchError(), even though their caller will also > report the error. Also, if a migration is cancelled early > because a uri was not set, we did not guarantee that the finish > stage would not overwrite the first error message. > > * src/qemu/qemu_migration.c (doPeer2PeerMigrate2) > (doPeer2PeerMigrate3): Preserve first error when cancelling. > * src/libvirt.c (virDomainMigrateVersion3Full): Likewise. > (virDomainMigrateVersion1, virDomainMigrateVersion2) > (virDomainMigrateDirect): Avoid redundant error dispatch. > (virDomainMigrateFinish2, virDomainMigrateFinish3) > (virDomainMigrateFinish3Params): Don't report error on cleanup > path. > (virDomainMigratePeer2PeerFull, virDomainMigrateDirect) > (virDomainMigrate2): Use correct errors. > (virDomainMigrate*): Prefer virReportError over virLibConnError. > > Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; > --- > src/libvirt.c | 181 ++++++++++++++++++++++++---------------------- > src/qemu/qemu_migration.c | 10 ++- > 2 files changed, 104 insertions(+), 87 deletions(-) > > diff --git a/src/libvirt.c b/src/libvirt.c > index a74bfc7..637bfc1 100644 > --- a/src/libvirt.c > +++ b/src/libvirt.c <...snip...> > > @@ -6387,7 +6396,7 @@ virDomainMigrateFinish2(virConnectPtr dconn, > cookie, cookielen, > uri, flags, > retcode); > - if (!ret) > + if (!ret && !retcode) This one (and the two below) feel like a bug fix unrelated to error reporting/processing...

ACK in general - your call on whether to split out the extra checks pointed out above or leave things as they are. If in fact those are "bugs" that "should" go into a RHEL release, then perhaps they need to be separated...