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@redhat.com> --- 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