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@xxxxxxxxxx> > --- > 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... > goto error; > return ret; > } > @@ -6679,7 +6688,7 @@ virDomainMigrateFinish3(virConnectPtr dconn, > cookieout, cookieoutlen, > dconnuri, uri, flags, > cancelled); > - if (!ret) > + if (!ret && !cancelled) here.. > goto error; > return ret; > } > @@ -6954,7 +6963,7 @@ virDomainMigrateFinish3Params(virConnectPtr dconn, > ret = dconn->driver->domainMigrateFinish3Params( > dconn, params, nparams, cookiein, cookieinlen, > cookieout, cookieoutlen, flags, cancelled); > - if (!ret) > + if (!ret && !cancelled) here... > goto error; > return ret; > } 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... John -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list