Re: [libvirt] [PATCHv2 10/14] Exit early after domain crash in monitor on migration

13 Jan 2015

<no commit message>

On 01/07/2015 10:42 AM, Ján Tomko wrote:
...
---
 src/qemu/qemu_migration.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 77e0b35..31494c8 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1663,7 +1663,8 @@ qemuMigrationStartNBDServer(virQEMUDriverPtr driver,
             qemuDomainObjExitMonitor(driver, vm);
Missing the error check here (yes, it's in 11/14)
...
goto cleanup;
         }
-        qemuDomainObjExitMonitor(driver, vm);
+        if (qemuDomainObjExitMonitor(driver, vm) < 0)
+            goto cleanup;
     }
priv->nbdPort = port;
@@ -1860,7 +1861,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
 }
-static void
+static int
 qemuMigrationStopNBDServer(virQEMUDriverPtr driver,
                            virDomainObjPtr vm,
                            qemuMigrationCookiePtr mig)
@@ -1868,22 +1869,23 @@ qemuMigrationStopNBDServer(virQEMUDriverPtr driver,
     qemuDomainObjPrivatePtr priv = vm->privateData;
if (!mig->nbd)
-        return;
+        return 0;
if (qemuDomainObjEnterMonitorAsync(driver, vm,
                                        QEMU_ASYNC_JOB_MIGRATION_IN) < 0)
-        return;
+        return -1;
if (qemuMonitorNBDServerStop(priv->mon) < 0)
         VIR_WARN("Unable to stop NBD server");
-    qemuDomainObjExitMonitor(driver, vm);
-
Why not ret = qemuDomainObjExitMonitor(), then return ret later on...

Holding the lock while virPortAllocatorRelease executes seems unnecessary...
...
virPortAllocatorRelease(driver->migrationPorts, priv->nbdPort);
     priv->nbdPort = 0;
+    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        return -1;
+    return 0;
 }
-static void
+static int
 qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
                                virQEMUDriverPtr driver,
                                virDomainObjPtr vm)
@@ -1891,6 +1893,7 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
     qemuDomainObjPrivatePtr priv = vm->privateData;
     size_t i;
     char *diskAlias = NULL;
+    int ret = 0;
VIR_DEBUG("mig=%p nbdPort=%d", mig->nbd, priv->nbdPort);
@@ -1914,12 +1917,15 @@ qemuMigrationCancelDriveMirror(qemuMigrationCookiePtr mig,
         if (qemuMonitorBlockJob(priv->mon, diskAlias, NULL, NULL, 0,
                                 BLOCK_JOB_ABORT, true) < 0)
             VIR_WARN("Unable to stop block job on %s", diskAlias);
-        qemuDomainObjExitMonitor(driver, vm);
+        if (qemuDomainObjExitMonitor(driver, vm) < 0) {
+            ret = -1;
+            goto cleanup;
+        }
     }
cleanup:
     VIR_FREE(diskAlias);
-    return;
+    return ret;
 }
/* Validate whether the domain is safe to migrate.  If vm is NULL,
@@ -2125,7 +2131,8 @@ qemuMigrationSetCompression(virQEMUDriverPtr driver,
                 state);
cleanup:
-    qemuDomainObjExitMonitor(driver, vm);
+    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        ret = -1;
or ret = qemuDomainObjExitMonitor();
   return ret;

(your choice here and the next few)

The rest seems OK... Might be nice to mention in a commit message why
failure to cancel drive mirror is ignorable while failure to stop nbd
server is not.

ACK, but it might be nice to pull in part of 11/14 since you're changing
ExitMonitor checks anyway

John
...
return ret;
 }
@@ -2164,7 +2171,8 @@ qemuMigrationSetAutoConverge(virQEMUDriverPtr driver,
                 state);
cleanup:
-    qemuDomainObjExitMonitor(driver, vm);
+    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        ret = -1;
     return ret;
 }
@@ -2210,7 +2218,8 @@ qemuMigrationSetPinAll(virQEMUDriverPtr driver,
                 state);
cleanup:
-    qemuDomainObjExitMonitor(driver, vm);
+    if (qemuDomainObjExitMonitor(driver, vm) < 0)
+        ret = -1;
     return ret;
 }
@@ -2476,7 +2485,8 @@ qemuDomainMigrateGraphicsRelocate(virQEMUDriverPtr driver,
                                        QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
         ret = qemuMonitorGraphicsRelocate(priv->mon, type, listenAddress,
                                           port, tlsPort, tlsSubject);
-        qemuDomainObjExitMonitor(driver, vm);
+        if (qemuDomainObjExitMonitor(driver, vm) < 0)
+            ret = -1;
     }
cleanup:
@@ -4000,7 +4010,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
/* cancel any outstanding NBD jobs */
     if (mig)
-        qemuMigrationCancelDriveMirror(mig, driver, vm);
+        ignore_value(qemuMigrationCancelDriveMirror(mig, driver, vm));
if (spec->fwdType != MIGRATION_FWD_DIRECT) {
         if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0)
@@ -5142,7 +5152,8 @@ qemuMigrationFinish(virQEMUDriverPtr driver,
                     VIR_WARN("unable to provide network data for relocation");
         }
-        qemuMigrationStopNBDServer(driver, vm, mig);
+        if (qemuMigrationStopNBDServer(driver, vm, mig) < 0)
+            goto endjob;
if (flags & VIR_MIGRATE_PERSIST_DEST) {
             virDomainDefPtr vmdef;


    

Re: [libvirt] [PATCHv2 10/14] Exit early after domain crash in monitor on migration

John Ferlan