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
@@ -4452,8 +4452,8 @@ virDomainMigrateVersion1(virDomainPtr domain,
goto done;
if (uri == NULL && uri_out == NULL) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare did not set uri"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare did not set uri"));
goto done;
}
if (uri_out)
@@ -4544,8 +4544,7 @@ virDomainMigrateVersion2(virDomainPtr domain,
* and pass it to Prepare2.
*/
if (!domain->conn->driver->domainGetXMLDesc) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
- virDispatchError(domain->conn);
+ virReportUnsupportedError();
return NULL;
}
@@ -4575,10 +4574,11 @@ virDomainMigrateVersion2(virDomainPtr domain,
goto done;
if (uri == NULL && uri_out == NULL) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare2 did not set uri"));
- virDispatchError(domain->conn);
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare2 did not set uri"));
cancelled = 1;
+ /* Make sure Finish doesn't overwrite the error */
+ orig_err = virSaveLastError();
goto finish;
}
if (uri_out)
@@ -4609,6 +4609,8 @@ finish:
VIR_DEBUG("Finish2 %p ret=%d", dconn, ret);
ddomain = dconn->driver->domainMigrateFinish2
(dconn, dname, cookie, cookielen, uri, destflags, cancelled);
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
done:
if (orig_err) {
@@ -4697,7 +4699,7 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
!domain->conn->driver->domainMigrateConfirm3Params ||
!dconn->driver->domainMigratePrepare3Params ||
!dconn->driver->domainMigrateFinish3Params))) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+ virReportUnsupportedError();
return NULL;
}
@@ -4771,13 +4773,19 @@ virDomainMigrateVersion3Full(virDomainPtr domain,
if (useParams &&
virTypedParamsReplaceString(¶ms, &nparams,
VIR_MIGRATE_PARAM_URI,
- uri_out) < 0)
+ uri_out) < 0) {
+ cancelled = 1;
+ orig_err = virSaveLastError();
goto finish;
+ }
} else if (!uri &&
virTypedParamsGetString(params, nparams,
VIR_MIGRATE_PARAM_URI, &uri) <= 0) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("domainMigratePrepare3 did not set uri"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("domainMigratePrepare3 did not set uri"));
+ cancelled = 1;
+ orig_err = virSaveLastError();
+ goto finish;
}
if (flags & VIR_MIGRATE_OFFLINE) {
@@ -4856,6 +4864,8 @@ finish:
(dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen,
NULL, uri, destflags, cancelled);
}
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
/* If ddomain is NULL, then we were unable to start
* the guest on the target, and must restart on the
@@ -4984,7 +4994,7 @@ virDomainMigratePeer2PeerFull(virDomainPtr domain,
(!useParams &&
!domain->conn->driver->domainMigratePerform &&
!domain->conn->driver->domainMigratePerform3)) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, __FUNCTION__);
+ virReportUnsupportedError();
return -1;
}
@@ -5013,14 +5023,14 @@ virDomainMigratePeer2PeerFull(virDomainPtr domain,
} else {
VIR_DEBUG("Using migration protocol 2");
if (xmlin) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Unable to change target guest XML "
- "during migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during "
+ "migration"));
return -1;
}
if (uri) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to override peer2peer migration URI"));
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+ _("Unable to override peer2peer migration URI"));
return -1;
}
return domain->conn->driver->domainMigratePerform
@@ -5081,7 +5091,6 @@ virDomainMigrateDirect(virDomainPtr domain,
if (!domain->conn->driver->domainMigratePerform) {
virReportUnsupportedError();
- virDispatchError(domain->conn);
return -1;
}
@@ -5107,8 +5116,8 @@ virDomainMigrateDirect(virDomainPtr domain,
} else {
VIR_DEBUG("Using migration protocol 2");
if (xmlin) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to change target guest XML during migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during migration"));
return -1;
}
return domain->conn->driver->domainMigratePerform(domain,
@@ -5236,16 +5245,16 @@ virDomainMigrate(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the source host"));
goto error;
}
if (!VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the destination host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the destination host"));
goto error;
}
}
@@ -5283,14 +5292,14 @@ virDomainMigrate(virDomainPtr domain,
if (flags & VIR_MIGRATE_CHANGE_PROTECTION &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("cannot enforce change protection"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("cannot enforce change protection"));
goto error;
}
flags &= ~VIR_MIGRATE_CHANGE_PROTECTION;
if (flags & VIR_MIGRATE_TUNNELLED) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot perform tunnelled migration without using peer2peer flag"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot perform tunnelled migration without using peer2peer flag"));
goto error;
}
@@ -5462,16 +5471,16 @@ virDomainMigrate2(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the source host"));
goto error;
}
if (!VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the destination host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the destination host"));
goto error;
}
}
@@ -5506,14 +5515,14 @@ virDomainMigrate2(virDomainPtr domain,
if (flags & VIR_MIGRATE_CHANGE_PROTECTION &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("cannot enforce change protection"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("cannot enforce change protection"));
goto error;
}
flags &= ~VIR_MIGRATE_CHANGE_PROTECTION;
if (flags & VIR_MIGRATE_TUNNELLED) {
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("cannot perform tunnelled migration without using peer2peer flag"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("cannot perform tunnelled migration without using peer2peer flag"));
goto error;
}
@@ -5531,8 +5540,8 @@ virDomainMigrate2(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V2)) {
VIR_DEBUG("Using migration protocol 2");
if (dxml) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to change target guest XML during migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during migration"));
goto error;
}
ddomain = virDomainMigrateVersion2(domain, dconn, flags,
@@ -5543,8 +5552,8 @@ virDomainMigrate2(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V1)) {
VIR_DEBUG("Using migration protocol 1");
if (dxml) {
- virLibConnError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("Unable to change target guest XML during migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during migration"));
goto error;
}
ddomain = virDomainMigrateVersion1(domain, dconn, flags,
@@ -5645,16 +5654,16 @@ virDomainMigrate3(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the source host"));
goto error;
}
if (!VIR_DRV_SUPPORTS_FEATURE(dconn->driver, dconn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the destination host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the destination host"));
goto error;
}
}
@@ -5667,8 +5676,8 @@ virDomainMigrate3(virDomainPtr domain,
if (flags & VIR_MIGRATE_CHANGE_PROTECTION &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATE_CHANGE_PROTECTION)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("cannot enforce change protection"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("cannot enforce change protection"));
goto error;
}
flags &= ~VIR_MIGRATE_CHANGE_PROTECTION;
@@ -5687,9 +5696,9 @@ virDomainMigrate3(virDomainPtr domain,
if (!virTypedParamsCheck(params, nparams, compatParams,
ARRAY_CARDINALITY(compatParams))) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Migration APIs with extensible parameters are not "
- "supported but extended parameters were passed"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Migration APIs with extensible parameters are not "
+ "supported but extended parameters were passed"));
goto error;
}
@@ -5717,9 +5726,9 @@ virDomainMigrate3(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V2)) {
VIR_DEBUG("Using migration protocol 2");
if (dxml) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Unable to change target guest XML during "
- "migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during "
+ "migration"));
goto error;
}
ddomain = virDomainMigrateVersion2(domain, dconn, flags,
@@ -5730,9 +5739,9 @@ virDomainMigrate3(virDomainPtr domain,
VIR_DRV_FEATURE_MIGRATION_V1)) {
VIR_DEBUG("Using migration protocol 1");
if (dxml) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Unable to change target guest XML during "
- "migration"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Unable to change target guest XML during "
+ "migration"));
goto error;
}
ddomain = virDomainMigrateVersion1(domain, dconn, flags,
@@ -5856,9 +5865,9 @@ virDomainMigrateToURI(virDomainPtr domain,
if (flags & VIR_MIGRATE_OFFLINE &&
!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_OFFLINE)) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("offline migration is not supported by "
- "the source host"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("offline migration is not supported by "
+ "the source host"));
goto error;
}
@@ -5883,9 +5892,9 @@ virDomainMigrateToURI(virDomainPtr domain,
goto error;
} else {
/* Cannot do a migration with only the perform step */
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("direct migration is not supported by the"
- " connection driver"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("direct migration is not supported by the"
+ " connection driver"));
goto error;
}
}
@@ -6031,9 +6040,9 @@ virDomainMigrateToURI2(virDomainPtr domain,
goto error;
} else {
/* Cannot do a migration with only the perform step */
- virLibConnError(VIR_ERR_OPERATION_INVALID, "%s",
- _("direct migration is not supported by the"
- " connection driver"));
+ virReportError(VIR_ERR_OPERATION_INVALID, "%s",
+ _("direct migration is not supported by the"
+ " connection driver"));
goto error;
}
}
@@ -6136,9 +6145,9 @@ virDomainMigrateToURI3(virDomainPtr domain,
if (flags & VIR_MIGRATE_PEER2PEER) {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_P2P)) {
- virLibConnError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("Peer-to-peer migration is not supported by "
- "the connection driver"));
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Peer-to-peer migration is not supported by "
+ "the connection driver"));
goto error;
}
@@ -6154,26 +6163,26 @@ virDomainMigrateToURI3(virDomainPtr domain,
dconnuri, uri, bandwidth) < 0)
goto error;
} else {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Peer-to-peer migration with extensible "
- "parameters is not supported but extended "
- "parameters were passed"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Peer-to-peer migration with extensible "
+ "parameters is not supported but extended "
+ "parameters were passed"));
goto error;
}
} else {
if (!VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn,
VIR_DRV_FEATURE_MIGRATION_DIRECT)) {
/* Cannot do a migration with only the perform step */
- virLibConnError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
- _("Direct migration is not supported by the"
- " connection driver"));
+ virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
+ _("Direct migration is not supported by the"
+ " connection driver"));
goto error;
}
if (!compat) {
- virLibConnError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
- _("Direct migration does not support extensible "
- "parameters"));
+ virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s",
+ _("Direct migration does not support extensible "
+ "parameters"));
goto error;
}
@@ -6387,7 +6396,7 @@ virDomainMigrateFinish2(virConnectPtr dconn,
cookie, cookielen,
uri, flags,
retcode);
- if (!ret)
+ if (!ret && !retcode)
goto error;
return ret;
}
@@ -6679,7 +6688,7 @@ virDomainMigrateFinish3(virConnectPtr dconn,
cookieout, cookieoutlen,
dconnuri, uri, flags,
cancelled);
- if (!ret)
+ if (!ret && !cancelled)
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)
goto error;
return ret;
}
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 9342062..8344a45 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3569,6 +3569,7 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver,
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domainMigratePrepare2 did not set uri"));
cancelled = true;
+ orig_err = virSaveLastError();
goto finish;
}
@@ -3608,6 +3609,8 @@ finish:
(dconn, dname, cookie, cookielen,
uri_out ? uri_out : dconnuri, destflags, cancelled);
qemuDomainObjExitRemote(vm);
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
cleanup:
if (ddomain) {
@@ -3769,11 +3772,14 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver,
uri = uri_out;
if (useParams &&
virTypedParamsReplaceString(¶ms, &nparams,
- VIR_MIGRATE_PARAM_URI, uri_out) < 0)
+ VIR_MIGRATE_PARAM_URI, uri_out) < 0) {
+ orig_err = virSaveLastError();
goto finish;
+ }
} else if (!uri && !(flags & VIR_MIGRATE_TUNNELLED)) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("domainMigratePrepare3 did not set uri"));
+ orig_err = virSaveLastError();
goto finish;
}
@@ -3850,6 +3856,8 @@ finish:
dconnuri, uri, destflags, cancelled);
qemuDomainObjExitRemote(vm);
}
+ if (cancelled && ddomain)
+ VIR_ERROR(_("finish step ignored that migration was cancelled"));
/* If ddomain is NULL, then we were unable to start
* the guest on the target, and must restart on the
--
1.8.4.2
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list