[libvirt] [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently

[Jiri Denemark]

On Tue, Oct 15, 2013 at 04:24:14 +0000, Wangyufei (A) wrote:
> From f56b290eab36bbb7a9ac53778a55638d473504d1 Mon Sep 17 00:00:00 2001
> From: WangYufei <james.wangyufei at huawei.com>
> Date: Fri, 11 Oct 2013 11:27:13 +0800
> Subject: [PATCH] qemu_migrate: Fix assign the same port when migrating concurrently
> 
> When we migrate vms concurrently, there's a chance that libvirtd on destination assign the same port for different migrations, which will lead to migration failed during migration prepare phase on destination. So we use virPortAllocator here to solve the problem.
> 
> Signed-off-by: WangYufei <james.wangyufei at huawei.com>
> ---
>  src/qemu/qemu_command.h   |    3 +++
>  src/qemu/qemu_conf.h      |    6 +++---
>  src/qemu/qemu_domain.h    |    1 +
>  src/qemu/qemu_driver.c    |    6 ++++++
>  src/qemu/qemu_migration.c |   28 +++++++++++++++++++++-------
>  5 files changed, 34 insertions(+), 10 deletions(-)
> 
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 3a1aab7..93ae237 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
...
> @@ -2297,6 +2300,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
>  
>      *def = NULL;
>      priv = vm->privateData;
> +    priv->migrationPort = port;
>      if (VIR_STRDUP(priv->origname, origname) < 0)
>          goto cleanup;
>  
> @@ -2415,6 +2419,11 @@ cleanup:
>      VIR_FREE(xmlout);
>      VIR_FORCE_CLOSE(dataFD[0]);
>      VIR_FORCE_CLOSE(dataFD[1]);
> +    if (ret < 0) {
> +        virPortAllocatorRelease(driver->migrationPorts, port);
> +        if (priv)
> +            priv->migrationPort = 0;
> +    }

This would possibly release a port that was not acquired from port
allocator but was supplied via migration URI.

>      if (vm) {
>          if (ret >= 0 || vm->persistent)
>              virObjectUnlock(vm);
...
> @@ -2600,8 +2609,11 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
>  cleanup:
>      virURIFree(uri);
>      VIR_FREE(hostname);
> -    if (ret != 0)
> +    if (ret != 0) {
>          VIR_FREE(*uri_out);
> +        virPortAllocatorRelease(driver->migrationPorts,
> +                                (unsigned short)this_port);
> +    }

And this may result in the port being released twice (once in
qemuMigrationPrepareAny and once here).

>      return ret;
>  }
>  

In other words, I think we need the following changes to be squashed in
(I'll send the complete patch with the suggested changes squashed).

Jirka

diff --git i/src/qemu/qemu_migration.c w/src/qemu/qemu_migration.c
index f8315da..0f17d0e 100644
--- i/src/qemu/qemu_migration.c
+++ w/src/qemu/qemu_migration.c
@@ -2159,7 +2159,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
                         virDomainDefPtr *def,
                         const char *origname,
                         virStreamPtr st,
-                        unsigned int port,
+                        unsigned short port,
+                        bool autoPort,
                         const char *listenAddress,
                         unsigned long flags)
 {
@@ -2328,7 +2329,6 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver,
 
     *def = NULL;
     priv = vm->privateData;
-    priv->migrationPort = port;
     if (VIR_STRDUP(priv->origname, origname) < 0)
         goto cleanup;
 
@@ -2440,6 +2440,8 @@ done:
         goto cleanup;
     }
 
+    if (autoPort)
+        priv->migrationPort = port;
     ret = 0;
 
 cleanup:
@@ -2447,11 +2449,6 @@ cleanup:
     VIR_FREE(xmlout);
     VIR_FORCE_CLOSE(dataFD[0]);
     VIR_FORCE_CLOSE(dataFD[1]);
-    if (ret < 0) {
-        virPortAllocatorRelease(driver->migrationPorts, port);
-        if (priv)
-            priv->migrationPort = 0;
-    }
     if (vm) {
         if (ret >= 0 || vm->persistent)
             virObjectUnlock(vm);
@@ -2531,7 +2528,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
                            const char *listenAddress,
                            unsigned long flags)
 {
-    int this_port;
+    unsigned short port = 0;
+    bool autoPort = true;
     char *hostname = NULL;
     const char *p;
     char *uri_str = NULL;
@@ -2558,8 +2556,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
      * to be a correct hostname which refers to the target machine).
      */
     if (uri_in == NULL) {
-        if (virPortAllocatorAcquire(driver->migrationPorts,
-                                    (unsigned short *)&this_port) < 0)
+        if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
             goto cleanup;
 
         /* Get hostname */
@@ -2616,8 +2613,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
 
         if (uri->port == 0) {
             /* Generate a port */
-            if (virPortAllocatorAcquire(driver->migrationPorts,
-                                        (unsigned short *)&this_port) < 0)
+            if (virPortAllocatorAcquire(driver->migrationPorts, &port) < 0)
                 goto cleanup;
 
             /* Caller frees */
@@ -2625,7 +2621,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
                 goto cleanup;
 
         } else {
-            this_port = uri->port;
+            port = uri->port;
+            autoPort = false;
         }
     }
 
@@ -2634,14 +2631,14 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver,
 
     ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen,
                                   cookieout, cookieoutlen, def, origname,
-                                  NULL, this_port, listenAddress, flags);
+                                  NULL, port, autoPort, listenAddress, flags);
 cleanup:
     virURIFree(uri);
     VIR_FREE(hostname);
     if (ret != 0) {
         VIR_FREE(*uri_out);
-        virPortAllocatorRelease(driver->migrationPorts,
-                                (unsigned short)this_port);
+        if (autoPort)
+            virPortAllocatorRelease(driver->migrationPorts, port);
     }
     return ret;
 }