Fix the signature of virDomainMigrateFinish3 for error reporting

The current virDomainMigrateFinish3 method signature attempts to
distinguish two types of errors, by allowing return with ret== 0,
but ddomain == NULL, to indicate a failure to start the guest.
This is flawed, because when ret == 0, there is no way for the
virErrorPtr details to be sent back to the client.

Change the signature of virDomainMigrateFinish3 so it simply
returns a virDomainPtr, in the same way as virDomainMigrateFinish2
The disk locking code will protect against the only possible
failure mode this doesn't account for (loosing conenctivity to
libvirtd after Finish3 starts the CPUs, but before the client
sees the reply for Finish3).

* src/driver.h, src/libvirt.c, src/libvirt_internal.h: Change
  virDomainMigrateFinish3 to return a virDomainPtr instead of int
* src/remote/remote_driver.c, src/remote/remote_protocol.x,
  daemon/remote.c, src/qemu/qemu_driver.c, src/qemu/qemu_migration.c:
  Update for API change

diff --git a/daemon/remote.c b/daemon/remote.c
index f85d760b37bfe8e8c63d9e66445961d0b9bb76c4..35129aacd2c57f50caffc8b631a351204b701262 100644 (file)
--- a/daemon/remote.c
+++ b/daemon/remote.c
@@ -76,7 +76,6 @@ static virStorageVolPtr get_nonnull_storage_vol(virConnectPtr conn, remote_nonnu
 static virSecretPtr get_nonnull_secret(virConnectPtr conn, remote_nonnull_secret secret);
 static virNWFilterPtr get_nonnull_nwfilter(virConnectPtr conn, remote_nonnull_nwfilter nwfilter);
 static virDomainSnapshotPtr get_nonnull_domain_snapshot(virDomainPtr dom, remote_nonnull_domain_snapshot snapshot);
-static int make_domain(remote_domain *dom_dst, virDomainPtr dom_src);
 static void make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src);
 static void make_nonnull_network(remote_nonnull_network *net_dst, virNetworkPtr net_src);
 static void make_nonnull_interface(remote_nonnull_interface *interface_dst, virInterfacePtr interface_src);
@@ -3359,19 +3358,16 @@ remoteDispatchDomainMigrateFinish3(struct qemud_server *server ATTRIBUTE_UNUSED,
     uri = args->uri == NULL ? NULL : *args->uri;
     dconnuri = args->dconnuri == NULL ? NULL : *args->dconnuri;
 
-    if (virDomainMigrateFinish3(conn, args->dname,
-                                args->cookie_in.cookie_in_val,
-                                args->cookie_in.cookie_in_len,
-                                &cookieout, &cookieoutlen,
-                                dconnuri, uri,
-                                args->flags,
-                                args->cancelled,
-                                &dom) < 0)
+    if (!(dom = virDomainMigrateFinish3(conn, args->dname,
+                                        args->cookie_in.cookie_in_val,
+                                        args->cookie_in.cookie_in_len,
+                                        &cookieout, &cookieoutlen,
+                                        dconnuri, uri,
+                                        args->flags,
+                                        args->cancelled)))
         goto cleanup;
 
-    if (dom &&
-        make_domain(&ret->ddom, dom) < 0)
-        goto cleanup;
+    make_nonnull_domain(&ret->dom, dom);
 
     /* remoteDispatchClientRequest will free cookie
      */
@@ -3493,21 +3489,6 @@ get_nonnull_domain_snapshot(virDomainPtr dom, remote_nonnull_domain_snapshot sna
 }
 
 /* Make remote_nonnull_domain and remote_nonnull_network. */
-static int
-make_domain(remote_domain *dom_dst, virDomainPtr dom_src)
-{
-    remote_domain rdom;
-    if (VIR_ALLOC(rdom) < 0)
-        return -1;
-
-    rdom->id = dom_src->id;
-    rdom->name = strdup(dom_src->name);
-    memcpy(rdom->uuid, dom_src->uuid, VIR_UUID_BUFLEN);
-
-    *dom_dst = rdom;
-    return 0;
-}
-
 static void
 make_nonnull_domain(remote_nonnull_domain *dom_dst, virDomainPtr dom_src)
 {

diff --git a/src/driver.h b/src/driver.h
index a5d8fe5fa5f7aca1e30087a7dca64f0052bb52bd..875ffcb3366b63483139e5cfc5c50c63eb9c40a1 100644 (file)
--- a/src/driver.h
+++ b/src/driver.h
@@ -587,7 +587,7 @@ typedef int
                      const char *dname,
                      unsigned long resource);
 
-typedef int
+typedef virDomainPtr
     (*virDrvDomainMigrateFinish3)
                     (virConnectPtr dconn,
                      const char *dname,
@@ -598,8 +598,7 @@ typedef int
                      const char *dconnuri,
                      const char *uri,
                      unsigned long flags,
-                     int cancelled,
-                     virDomainPtr *newdom);
+                     int cancelled);
 
 typedef int
     (*virDrvDomainMigrateConfirm3)

diff --git a/src/libvirt.c b/src/libvirt.c
index 8f9b6cebda0e44d666540637e90d8cfb723a749f..39c32136648efee944fb37602965a0d691b91432 100644 (file)
--- a/src/libvirt.c
+++ b/src/libvirt.c
@@ -3829,18 +3829,19 @@ finish:
     cookieout = NULL;
     cookieoutlen = 0;
     dname = dname ? dname : domain->name;
-    ret = dconn->driver->domainMigrateFinish3
+    ddomain = dconn->driver->domainMigrateFinish3
         (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen,
-         NULL, uri, flags, cancelled, &ddomain);
-
-    /* If ret is 0 then 'ddomain' indicates whether the VM is
-     * running on the dest. If not running, we can restart
-     * the source.  If ret is -1, we can't be sure what happened
-     * to the VM on the dest, thus the only safe option is to
-     * kill the VM on the source, even though that may leave
-     * no VM at all on either host.
+         NULL, uri, flags, cancelled);
+
+    /* If ddomain is NULL, then we were unable to start
+     * the guest on the target, and must restart on the
+     * source. There is a small chance that the ddomain
+     * is NULL due to an RPC failure, in which case
+     * ddomain could in fact be running on the dest.
+     * The lock manager plugins should take care of
+     * safety in this scenario.
      */
-    cancelled = ret == 0 && ddomain == NULL ? 1 : 0;
+    cancelled = ddomain == NULL ? 1 : 0;
 
     /* If finish3 set an error, and we don't have an earlier
      * one we need to preserve it in case confirm3 overwrites
@@ -5158,7 +5159,7 @@ error:
  * Not for public use.  This function is part of the internal
  * implementation of migration in the remote case.
  */
-int
+virDomainPtr
 virDomainMigrateFinish3(virConnectPtr dconn,
                         const char *dname,
                         const char *cookiein,
@@ -5168,20 +5169,19 @@ virDomainMigrateFinish3(virConnectPtr dconn,
                         const char *dconnuri,
                         const char *uri,
                         unsigned long flags,
-                        int cancelled,
-                        virDomainPtr *newdom)
+                        int cancelled)
 {
     VIR_DEBUG("dconn=%p, dname=%s, cookiein=%p, cookieinlen=%d, cookieout=%p,"
-              "cookieoutlen=%p, dconnuri=%s, uri=%s, flags=%lu, retcode=%d newdom=%p",
+              "cookieoutlen=%p, dconnuri=%s, uri=%s, flags=%lu, retcode=%d",
               dconn, NULLSTR(dname), cookiein, cookieinlen, cookieout,
-              cookieoutlen, NULLSTR(dconnuri), NULLSTR(uri), flags, cancelled, newdom);
+              cookieoutlen, NULLSTR(dconnuri), NULLSTR(uri), flags, cancelled);
 
     virResetLastError();
 
     if (!VIR_IS_CONNECT (dconn)) {
         virLibConnError(VIR_ERR_INVALID_CONN, __FUNCTION__);
         virDispatchError(NULL);
-        return -1;
+        return NULL;
     }
 
     if (dconn->flags & VIR_CONNECT_RO) {
@@ -5190,14 +5190,13 @@ virDomainMigrateFinish3(virConnectPtr dconn,
     }
 
     if (dconn->driver->domainMigrateFinish3) {
-        int ret;
+        virDomainPtr ret;
         ret = dconn->driver->domainMigrateFinish3(dconn, dname,
                                                   cookiein, cookieinlen,
                                                   cookieout, cookieoutlen,
                                                   dconnuri, uri, flags,
-                                                  cancelled,
-                                                  newdom);
-        if (ret < 0)
+                                                  cancelled);
+        if (!ret)
             goto error;
         return ret;
     }
@@ -5206,7 +5205,7 @@ virDomainMigrateFinish3(virConnectPtr dconn,
 
 error:
     virDispatchError(dconn);
-    return -1;
+    return NULL;
 }
 
 

diff --git a/src/libvirt_internal.h b/src/libvirt_internal.h
index 1d71a6cf52011c73e6c1ac1856b761bd7220181f..83c25fceb85a91e7573d8f010b9a764dbefc4be1 100644 (file)
--- a/src/libvirt_internal.h
+++ b/src/libvirt_internal.h
@@ -167,17 +167,16 @@ int virDomainMigratePerform3(virDomainPtr dom,
                              const char *dname,
                              unsigned long resource);
 
-int virDomainMigrateFinish3(virConnectPtr dconn,
-                            const char *dname,
-                            const char *cookiein,
-                            int cookieinlen,
-                            char **cookieout,
-                            int *cookieoutlen,
-                            const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */
-                            const char *uri, /* VM Migration URI, NULL in tunnelled case */
-                            unsigned long flags,
-                            int cancelled, /* Kill the dst VM */
-                            virDomainPtr *newdom);
+virDomainPtr virDomainMigrateFinish3(virConnectPtr dconn,
+                                     const char *dname,
+                                     const char *cookiein,
+                                     int cookieinlen,
+                                     char **cookieout,
+                                     int *cookieoutlen,
+                                     const char *dconnuri, /* libvirtd URI if Peer2Peer, NULL otherwise */
+                                     const char *uri, /* VM Migration URI, NULL in tunnelled case */
+                                     unsigned long flags,
+                                     int cancelled); /* Kill the dst VM */
 
 int virDomainMigrateConfirm3(virDomainPtr domain,
                              const char *cookiein,

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index df6e1c2bef5ca99d0527b57ba04c0ed2c9621be8..6511ffd9764b4d8764c37df8a6d28cbb35241906 100644 (file)
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -6232,7 +6232,7 @@ cleanup:
 }
 
 
-static int
+static virDomainPtr
 qemuDomainMigrateFinish3(virConnectPtr dconn,
                          const char *dname,
                          const char *cookiein,
@@ -6242,12 +6242,11 @@ qemuDomainMigrateFinish3(virConnectPtr dconn,
                          const char *dconnuri ATTRIBUTE_UNUSED,
                          const char *uri ATTRIBUTE_UNUSED,
                          unsigned long flags,
-                         int cancelled,
-                         virDomainPtr *newdom)
+                         int cancelled)
 {
     struct qemud_driver *driver = dconn->privateData;
     virDomainObjPtr vm;
-    int ret = -1;
+    virDomainPtr dom = NULL;
 
     virCheckFlags(VIR_MIGRATE_LIVE |
                   VIR_MIGRATE_PEER2PEER |
@@ -6256,7 +6255,7 @@ qemuDomainMigrateFinish3(virConnectPtr dconn,
                   VIR_MIGRATE_UNDEFINE_SOURCE |
                   VIR_MIGRATE_PAUSED |
                   VIR_MIGRATE_NON_SHARED_DISK |
-                  VIR_MIGRATE_NON_SHARED_INC, -1);
+                  VIR_MIGRATE_NON_SHARED_INC, NULL);
 
     qemuDriverLock(driver);
     vm = virDomainFindByName(&driver->domains, dname);
@@ -6266,16 +6265,14 @@ qemuDomainMigrateFinish3(virConnectPtr dconn,
         goto cleanup;
     }
 
-    *newdom = qemuMigrationFinish(driver, dconn, vm,
-                                  cookiein, cookieinlen,
-                                  cookieout, cookieoutlen,
-                                  flags, cancelled, true);
-
-    ret = 0;
+    dom = qemuMigrationFinish(driver, dconn, vm,
+                              cookiein, cookieinlen,
+                              cookieout, cookieoutlen,
+                              flags, cancelled, true);
 
 cleanup:
     qemuDriverUnlock(driver);
-    return ret;
+    return dom;
 }
 
 static int

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 660365568c3f0888f90a31d8c39f4b512701d045..2f0ed28765a4fe2cfb42f10419455432a28bb884 100644 (file)
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1942,19 +1942,20 @@ finish:
     cookieoutlen = 0;
     dname = dname ? dname : vm->def->name;
     qemuDomainObjEnterRemoteWithDriver(driver, vm);
-    ret = dconn->driver->domainMigrateFinish3
+    ddomain = dconn->driver->domainMigrateFinish3
         (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen,
-         dconnuri, uri_out ? uri_out : uri, flags, cancelled, &ddomain);
+         dconnuri, uri_out ? uri_out : uri, flags, cancelled);
     qemuDomainObjExitRemoteWithDriver(driver, vm);
 
-    /* If ret is 0 then 'ddomain' indicates whether the VM is
-     * running on the dest. If not running, we can restart
-     * the source.  If ret is -1, we can't be sure what happened
-     * to the VM on the dest, thus the only safe option is to
-     * kill the VM on the source, even though that may leave
-     * no VM at all on either host.
+    /* If ddomain is NULL, then we were unable to start
+     * the guest on the target, and must restart on the
+     * source. There is a small chance that the ddomain
+     * is NULL due to an RPC failure, in which case
+     * ddomain could in fact be running on the dest.
+     * The lock manager plugins should take care of
+     * safety in this scenario.
      */
-    cancelled = ret == 0 && ddomain == NULL ? 1 : 0;
+    cancelled = ddomain == NULL ? 1 : 0;
 
     /* If finish3 set an error, and we don't have an earlier
      * one we need to preserve it in case confirm3 overwrites

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index 64f56201fa29af2fdb0e263f982915361f0d25bc..14f99082708a9b65be1392f95bf25fed9761bdb7 100644 (file)
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -238,7 +238,6 @@ static int remoteAuthPolkit (virConnectPtr conn, struct private_data *priv, int
     virReportErrorHelper(VIR_FROM_REMOTE, code, __FILE__,         \
                          __FUNCTION__, __LINE__, __VA_ARGS__)
 
-static virDomainPtr get_domain (virConnectPtr conn, remote_domain domain);
 static virDomainPtr get_nonnull_domain (virConnectPtr conn, remote_nonnull_domain domain);
 static virNetworkPtr get_nonnull_network (virConnectPtr conn, remote_nonnull_network network);
 static virNWFilterPtr get_nonnull_nwfilter (virConnectPtr conn, remote_nonnull_nwfilter nwfilter);
@@ -5228,7 +5227,7 @@ error:
 }
 
 
-static int
+static virDomainPtr
 remoteDomainMigrateFinish3(virConnectPtr dconn,
                            const char *dname,
                            const char *cookiein,
@@ -5238,17 +5237,15 @@ remoteDomainMigrateFinish3(virConnectPtr dconn,
                            const char *dconnuri,
                            const char *uri,
                            unsigned long flags,
-                           int cancelled,
-                           virDomainPtr *ddom)
+                           int cancelled)
 {
     remote_domain_migrate_finish3_args args;
     remote_domain_migrate_finish3_ret ret;
     struct private_data *priv = dconn->privateData;
-    int rv = -1;
+    virDomainPtr rv = NULL;
 
     remoteDriverLock(priv);
 
-    *ddom = NULL;
     memset(&args, 0, sizeof(args));
     memset(&ret, 0, sizeof(ret));
 
@@ -5265,7 +5262,7 @@ remoteDomainMigrateFinish3(virConnectPtr dconn,
               (xdrproc_t) xdr_remote_domain_migrate_finish3_ret, (char *) &ret) == -1)
         goto done;
 
-    *ddom = get_domain(dconn, ret.ddom);
+    rv = get_nonnull_domain(dconn, ret.dom);
 
     if (ret.cookie_out.cookie_out_len > 0) {
         if (!cookieout || !cookieoutlen) {
@@ -5281,8 +5278,6 @@ remoteDomainMigrateFinish3(virConnectPtr dconn,
 
     xdr_free ((xdrproc_t) &xdr_remote_domain_migrate_finish3_ret, (char *) &ret);
 
-    rv = 0;
-
 done:
     remoteDriverUnlock(priv);
     return rv;
@@ -6608,22 +6603,6 @@ remoteDomainEventQueue(struct private_data *priv, virDomainEventPtr event)
     virDomainEventStateQueue(priv->domainEventState, event);
 }
 
-/* get_nonnull_domain and get_nonnull_network turn an on-wire
- * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
- * These can return NULL if underlying memory allocations fail,
- * but if they do then virterror_internal.has been set.
- */
-static virDomainPtr
-get_domain (virConnectPtr conn, remote_domain domain)
-{
-    virDomainPtr dom = NULL;
-    if (domain) {
-        dom = virGetDomain (conn, domain->name, BAD_CAST domain->uuid);
-        if (dom) dom->id = domain->id;
-    }
-    return dom;
-}
-
 /* get_nonnull_domain and get_nonnull_network turn an on-wire
  * (name, uuid) pair into virDomainPtr or virNetworkPtr object.
  * These can return NULL if underlying memory allocations fail,

diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
index 58afee02c9a6e65c00fcbf6112305d7b464578f5..d8c0e532d1791c88e3b0b267946d983be435f38c 100644 (file)
--- a/src/remote/remote_protocol.x
+++ b/src/remote/remote_protocol.x
@@ -2035,7 +2035,7 @@ struct remote_domain_migrate_finish3_args {
 };
 
 struct remote_domain_migrate_finish3_ret {
-    remote_domain ddom;
+    remote_nonnull_domain dom;
     opaque cookie_out<REMOTE_MIGRATE_COOKIE_MAX>;
 };
 

diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
index b6cef13315208f4771e18c55e3a35fdcccaf5abb..e78338e90d2d8e36bf50e889b94d4734a9258fa0 100644 (file)
--- a/src/remote_protocol-structs
+++ b/src/remote_protocol-structs
@@ -1534,7 +1534,7 @@ struct remote_domain_migrate_finish3_args {
         int                        cancelled;
 };
 struct remote_domain_migrate_finish3_ret {
-        remote_domain              ddom;
+        remote_nonnull_domain      dom;
         struct {
                 u_int              cookie_out_len;
                 char *             cookie_out_val;