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

[John Ferlan]

<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;
>