[PATCH 0/3] libxl: a few small migration fixes

[PATCH 0/3] libxl: a few small migration fixes

[Jim Fehlig]

This series fixes a few issues found while testing migration with
latest Xen and libvirt.  See the individual patches for details.

Jim Fehlig (3):
  libxl: fix ref counting of libxlMigrationDstArgs
  libxl: don't attempt to resume domain when suspend fails
  libxl: acquire a job when receiving a migrating domain

 src/libxl/libxl_migration.c | 52 +++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 30 deletions(-)

-- 
2.1.4

[PATCH 1/3] libxl: fix ref counting of libxlMigrationDstArgs

[Jim Fehlig]

This patch fixes some flawed logic around ref counting the
libxlMigrationDstArgs object.

First, when adding sockets to the event loop with
virNetSocketAddIOCallback(), the generic virObjectFreeCallback()
was registered as a free function, with libxlMigrationDstArgs as
its parameter. A reference was also taken on
libxlMigrationDstArgs for each successful call to
virNetSocketAddIOCallback(). The rational behind this logic was
that the libxlMigrationDstArgs object had to out-live the socket
objects. But virNetSocketAddIOCallback() already takes a
reference on socket objects, ensuring their life until removed
from the event loop and unref'ed in virNetSocketEventFree(). We
only need to ensure libxlMigrationDstArgs lives until
libxlDoMigrateReceive() finishes, which can be done by simply
unref'ing libxlMigrationDstArgs at the end of
libxlDoMigrateReceive().

The second flaw was unref'ing the sockets in the failure path of
libxlMigrateReceive() and at the end of libxlDoMigrateReceive().
As mentioned above, the sockets are already unref'ed by
virNetSocketEventFree() when removed from the event loop.
Attempting to unref the socket a second time resulted in a
libvirtd crash since the socket was previously unref'ed and
disposed.

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_migration.c | 18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index aa9547b..4349f02 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -112,11 +112,11 @@ libxlDoMigrateReceive(void *opaque)
         virNetSocketUpdateIOCallback(socks[i], 0);
         virNetSocketRemoveIOCallback(socks[i]);
         virNetSocketClose(socks[i]);
-        virObjectUnref(socks[i]);
         socks[i] = NULL;
     }
     args->nsocks = 0;
     VIR_FORCE_CLOSE(recvfd);
+    virObjectUnref(args);
 }
 
 
@@ -164,11 +164,11 @@ libxlMigrateReceive(virNetSocketPtr sock,
         virNetSocketUpdateIOCallback(socks[i], 0);
         virNetSocketRemoveIOCallback(socks[i]);
         virNetSocketClose(socks[i]);
-        virObjectUnref(socks[i]);
         socks[i] = NULL;
     }
     args->nsocks = 0;
     VIR_FORCE_CLOSE(recvfd);
+    virObjectUnref(args);
 }
 
 static int
@@ -420,22 +420,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
                                       VIR_EVENT_HANDLE_READABLE,
                                       libxlMigrateReceive,
                                       args,
-                                      virObjectFreeCallback) < 0)
+                                      NULL) < 0)
             continue;
 
-        /*
-         * Successfully added sock to event loop.  Take a ref on args to
-         * ensure it is not freed until sock is removed from the event loop.
-         * Ref is dropped in virObjectFreeCallback after being removed
-         * from the event loop.
-         */
-        virObjectRef(args);
         nsocks_listen++;
     }
 
-    /* Done with args in this function, drop reference */
-    virObjectUnref(args);
-
     if (!nsocks_listen)
         goto error;
 
@@ -448,6 +438,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
         virObjectUnref(socks[i]);
     }
     VIR_FREE(socks);
+    virObjectUnref(args);
+
     /* Remove virDomainObj from domain list */
     if (vm) {
         virDomainObjListRemove(driver->domains, vm);
-- 
2.1.4

Re: [PATCH 1/3] libxl: fix ref counting of libxlMigrationDstArgs

[Olaf Hering]

On Thu, Jul 16, Jim Fehlig wrote:

> @@ -448,6 +438,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>          virObjectUnref(socks[i]);
>      }
>      VIR_FREE(socks);
> +    virObjectUnref(args);

This is now below the 'error' label, so args has to be initialized.

[  149s] libxl/libxl_migration.c: In function 'libxlDomainMigrationPrepare':
[  149s] libxl/libxl_migration.c:463:19: warning: 'args' may be used uninitialized in this function [-Wmaybe-uninitialized]
[  149s]      virObjectUnref(args);
[  149s]                    ^

Olaf

Re: [PATCH 1/3] libxl: fix ref counting of libxlMigrationDstArgs

[Jim Fehlig]

On 07/20/2015 09:28 AM, Olaf Hering wrote:
> On Thu, Jul 16, Jim Fehlig wrote:
>
>> @@ -448,6 +438,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn,
>>           virObjectUnref(socks[i]);
>>       }
>>       VIR_FREE(socks);
>> +    virObjectUnref(args);
> This is now below the 'error' label, so args has to be initialized.
>
> [  149s] libxl/libxl_migration.c: In function 'libxlDomainMigrationPrepare':
> [  149s] libxl/libxl_migration.c:463:19: warning: 'args' may be used uninitialized in this function [-Wmaybe-uninitialized]
> [  149s]      virObjectUnref(args);
> [  149s]                    ^

Opps. I've squashed in the below obvious fix. Note it is safe to call 
virObjectUnref with a NULL object.

Any other comments on this series? It fixes bugs in migration, including a 
possible libvirtd crash, and would be a candidate to push during freeze IMO.

Regards,
Jim

[PATCH 2/3] libxl: don't attempt to resume domain when suspend fails

[Jim Fehlig]

Failure of libxl_domain_suspend() does not leave the domain in
a suspended state, so no need to call libxl_domain_resume(),
which btw will fail with "domain not suspended".

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---

AFAICT, the xl migration code does not call libxl_domain_resume()
when libxl_domain_suspend() fails either. Perhaps one of the Xen
tools maintainers can verify if calling libxl_domain_resume() is
unnecessary when libxl_domain_suspend() fails.

 src/libxl/libxl_migration.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 4349f02..2ac4869 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -178,7 +178,6 @@ libxlDoMigrateSend(libxlDriverPrivatePtr driver,
                    int sockfd)
 {
     libxlDriverConfigPtr cfg = libxlDriverConfigGet(driver);
-    virObjectEventPtr event = NULL;
     int xl_flags = 0;
     int ret;
 
@@ -188,24 +187,11 @@ libxlDoMigrateSend(libxlDriverPrivatePtr driver,
     ret = libxl_domain_suspend(cfg->ctx, vm->def->id, sockfd,
                                xl_flags, NULL);
     if (ret != 0) {
-        /* attempt to resume the domain on failure */
-        if (libxl_domain_resume(cfg->ctx, vm->def->id, 1, 0) != 0) {
-            VIR_DEBUG("Failed to resume domain following failed migration");
-            virDomainObjSetState(vm, VIR_DOMAIN_PAUSED,
-                                 VIR_DOMAIN_PAUSED_MIGRATION);
-            event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_SUSPENDED,
-                                             VIR_DOMAIN_EVENT_SUSPENDED_MIGRATED);
-            ignore_value(virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm));
-        }
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("Failed to send migration data to destination host"));
         ret = -1;
-        goto cleanup;
     }
 
- cleanup:
-    if (event)
-        libxlDomainEventQueue(driver, event);
     virObjectUnref(cfg);
     return ret;
 }
-- 
2.1.4

[PATCH 3/3] libxl: acquire a job when receiving a migrating domain

[Jim Fehlig]

Commit f86ae403 moved acquiring a job from libxlDomainStart()
to its callers. One spot missed was in libxlDoMigrateReceive().
Acquire a job in libxlDoMigrateReceive() before calling
libxlDomainStart().

Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
 src/libxl/libxl_migration.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
index 2ac4869..7bf62c6 100644
--- a/src/libxl/libxl_migration.c
+++ b/src/libxl/libxl_migration.c
@@ -95,17 +95,20 @@ libxlDoMigrateReceive(void *opaque)
     int recvfd = args->recvfd;
     size_t i;
     int ret;
+    bool remove_dom = 0;
+
+    virObjectLock(vm);
+    if (libxlDomainObjBeginJob(driver, vm, LIBXL_JOB_MODIFY) < 0)
+        goto cleanup;
 
     /*
      * Always start the domain paused.  If needed, unpause in the
      * finish phase, after transfer of the domain is complete.
      */
-    virObjectLock(vm);
     ret = libxlDomainStart(driver, vm, true, recvfd);
-    virObjectUnlock(vm);
 
     if (ret < 0 && !vm->persistent)
-        virDomainObjListRemove(driver->domains, vm);
+        remove_dom = true;
 
     /* Remove all listen socks from event handler, and close them. */
     for (i = 0; i < nsocks; i++) {
@@ -117,6 +120,17 @@ libxlDoMigrateReceive(void *opaque)
     args->nsocks = 0;
     VIR_FORCE_CLOSE(recvfd);
     virObjectUnref(args);
+
+    if (!libxlDomainObjEndJob(driver, vm))
+        vm = NULL;
+
+ cleanup:
+    if (remove_dom && vm) {
+        virDomainObjListRemove(driver->domains, vm);
+        vm = NULL;
+    }
+    if (vm)
+        virObjectUnlock(vm);
 }
 
 
-- 
2.1.4