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

Saturday, 28 December 2013

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
@@ -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(&params, &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(&params, &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


    

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

2007

2006

2005

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