8:56 a.m.
Mostly refactoring of the horrible mess in qemuMigrationRun.
Jiri Denemark (7):
qemu: Use switch in qemuMigrationCompleted
qemu: Refactor qemuMigrationRun a bit
qemu: Split cleanup and error code in qemuMigrationRun
qemu: Unite error handling in qemuMigrationRun
qemu: Don't misuse "ret" in qemuMigrationRun
qemu: Consistently use exit_monitor in qemuMigrationRun
qemu: Set correct job status when qemuMigrationRun fails
src/qemu/qemu_migration.c | 196 +++++++++++++++++++++++++---------------------
1 file changed, 105 insertions(+), 91 deletions(-)
--
2.14.2
8:56 a.m.
New subject: [libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted
When adding a new job state it's useful to let the compiler complain
about places where we need to think about what to do with the new
state.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 72edbb667..c3f9c38b2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
return 0;
error:
- /* state can not be active or completed at this point */
- if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
- jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
+ switch (jobInfo->status) {
+ case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
+ case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
/* The migration was aborted by us rather than QEMU itself. */
jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
return -2;
- } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
+
+ case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
+ /* Something failed after QEMU already finished the migration. */
jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
return -1;
- } else {
+
+ case QEMU_DOMAIN_JOB_STATUS_FAILED:
+ case QEMU_DOMAIN_JOB_STATUS_CANCELED:
+ /* QEMU aborted the migration. */
return -1;
+
+ case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+ case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
+ case QEMU_DOMAIN_JOB_STATUS_NONE:
+ /* Impossible. */
+ break;
}
+
+ return -1;
}
--
2.14.2
4:28 a.m.
New subject: [libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted
On Thu, Oct 19, 2017 at 03:56 PM +0200, Jiri Denemark <jdenemar(a)redhat.com> wrote:
When adding a new job state it's useful to let the compiler
complain
about places where we need to think about what to do with the new
state.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
1 file changed, 18 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 72edbb667..c3f9c38b2 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
return 0;
error:
- /* state can not be active or completed at this point */
- if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
- jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
+ switch (jobInfo->status) {
+ case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
+ case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
/* The migration was aborted by us rather than QEMU itself. */
jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
return -2;
- } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
+
+ case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
+ /* Something failed after QEMU already finished the migration. */
jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
return -1;
- } else {
+
+ case QEMU_DOMAIN_JOB_STATUS_FAILED:
+ case QEMU_DOMAIN_JOB_STATUS_CANCELED:
+ /* QEMU aborted the migration. */
return -1;
+
+ case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
+ case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
+ case QEMU_DOMAIN_JOB_STATUS_NONE:
+ /* Impossible. */
+ break;
}
+
+ return -1;
}
I think you have to add ATTRIBUTE_FALLTHROUGH for the intended
fallthroughs (e.g. for gcc 7).
--
2.14.2
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
5:12 a.m.
New subject: [libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted
On Fri, Oct 20, 2017 at 11:28:37 +0200, Marc Hartmayer wrote:
On Thu, Oct 19, 2017 at 03:56 PM +0200, Jiri Denemark
<jdenemar(a)redhat.com> wrote:
> When adding a new job state it's useful to let the compiler complain
> about places where we need to think about what to do with the new
> state.
>
> Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> ---
> src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
> 1 file changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 72edbb667..c3f9c38b2 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
> return 0;
>
> error:
> - /* state can not be active or completed at this point */
> - if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
> - jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
> + switch (jobInfo->status) {
> + case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
> + case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
> /* The migration was aborted by us rather than QEMU itself. */
> jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
> return -2;
> - } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
> +
> + case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
> + /* Something failed after QEMU already finished the migration. */
> jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
> return -1;
> - } else {
> +
> + case QEMU_DOMAIN_JOB_STATUS_FAILED:
> + case QEMU_DOMAIN_JOB_STATUS_CANCELED:
> + /* QEMU aborted the migration. */
> return -1;
> +
> + case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
> + case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
> + case QEMU_DOMAIN_JOB_STATUS_NONE:
> + /* Impossible. */
> + break;
> }
> +
> + return -1;
> }
I think you have to add ATTRIBUTE_FALLTHROUGH for the intended
fallthroughs (e.g. for gcc 7).
This should only be needed if there was any code between the cases.
Jirka
5:45 a.m.
New subject: [libvirt] [PATCH 1/7] qemu: Use switch in qemuMigrationCompleted
On Fri, Oct 20, 2017 at 12:12 PM +0200, Jiri Denemark <jdenemar(a)redhat.com> wrote:
On Fri, Oct 20, 2017 at 11:28:37 +0200, Marc Hartmayer wrote:
> On Thu, Oct 19, 2017 at 03:56 PM +0200, Jiri Denemark <jdenemar(a)redhat.com>
wrote:
> > When adding a new job state it's useful to let the compiler complain
> > about places where we need to think about what to do with the new
> > state.
> >
> > Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
> > ---
> > src/qemu/qemu_migration.c | 23 ++++++++++++++++++-----
> > 1 file changed, 18 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > index 72edbb667..c3f9c38b2 100644
> > --- a/src/qemu/qemu_migration.c
> > +++ b/src/qemu/qemu_migration.c
> > @@ -1531,18 +1531,31 @@ qemuMigrationCompleted(virQEMUDriverPtr driver,
> > return 0;
> >
> > error:
> > - /* state can not be active or completed at this point */
> > - if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
> > - jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
> > + switch (jobInfo->status) {
> > + case QEMU_DOMAIN_JOB_STATUS_MIGRATING:
> > + case QEMU_DOMAIN_JOB_STATUS_POSTCOPY:
> > /* The migration was aborted by us rather than QEMU itself. */
> > jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
> > return -2;
> > - } else if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
> > +
> > + case QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED:
> > + /* Something failed after QEMU already finished the migration. */
> > jobInfo->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
> > return -1;
> > - } else {
> > +
> > + case QEMU_DOMAIN_JOB_STATUS_FAILED:
> > + case QEMU_DOMAIN_JOB_STATUS_CANCELED:
> > + /* QEMU aborted the migration. */
> > return -1;
> > +
> > + case QEMU_DOMAIN_JOB_STATUS_ACTIVE:
> > + case QEMU_DOMAIN_JOB_STATUS_COMPLETED:
> > + case QEMU_DOMAIN_JOB_STATUS_NONE:
> > + /* Impossible. */
> > + break;
> > }
> > +
> > + return -1;
> > }
>
> I think you have to add ATTRIBUTE_FALLTHROUGH for the intended
> fallthroughs (e.g. for gcc 7).
This should only be needed if there was any code between the cases.
Thanks for the information.
Jirka
--
Beste Grüße / Kind regards
Marc Hartmayer
IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
8:56 a.m.
New subject: [libvirt] [PATCH 2/7] qemu: Refactor qemuMigrationRun a bit
Some code which was supposed to be executed only when migration
succeeded was buried inside the cleanup code.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 63 +++++++++++++++++++++++++++--------------------
1 file changed, 36 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index c3f9c38b2..f0d4f9d98 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3867,8 +3867,36 @@ qemuMigrationRun(virQEMUDriverPtr driver,
qemuMigrationSetOffline(driver, vm) < 0) {
goto cancelPostCopy;
}
- if (priv->job.completed)
+
+ if (mig && mig->nbd &&
+ qemuMigrationCancelDriveMirror(driver, vm, true,
+ QEMU_ASYNC_JOB_MIGRATION_OUT,
+ dconn) < 0)
+ goto cancelPostCopy;
+
+ if (iothread) {
+ qemuMigrationIOThreadPtr io;
+
+ VIR_STEAL_PTR(io, iothread);
+ if (qemuMigrationStopTunnel(io, false) < 0)
+ goto cancelPostCopy;
+ }
+
+ if (priv->job.completed) {
priv->job.completed->stopped = priv->job.current->stopped;
+ qemuDomainJobInfoUpdateTime(priv->job.completed);
+ qemuDomainJobInfoUpdateDowntime(priv->job.completed);
+ ignore_value(virTimeMillisNow(&priv->job.completed->sent));
+ }
+
+ cookieFlags |= QEMU_MIGRATION_COOKIE_NETWORK |
+ QEMU_MIGRATION_COOKIE_STATS;
+
+ if (qemuMigrationCookieAddPersistent(mig, &persistDef) < 0 ||
+ qemuMigrationBakeCookie(mig, driver, vm, cookieout,
+ cookieoutlen, cookieFlags) < 0) {
+ VIR_WARN("Unable to encode migration cookie");
+ }
ret = 0;
@@ -3877,43 +3905,24 @@ qemuMigrationRun(virQEMUDriverPtr driver,
orig_err = virSaveLastError();
/* cancel any outstanding NBD jobs */
- if (mig && mig->nbd) {
- if (qemuMigrationCancelDriveMirror(driver, vm, ret == 0,
- QEMU_ASYNC_JOB_MIGRATION_OUT,
- dconn) < 0)
- ret = -1;
- }
+ if (ret < 0 && mig && mig->nbd)
+ qemuMigrationCancelDriveMirror(driver, vm, false,
+ QEMU_ASYNC_JOB_MIGRATION_OUT,
+ dconn);
VIR_FREE(tlsAlias);
VIR_FREE(secAlias);
virObjectUnref(cfg);
- if (spec->fwdType != MIGRATION_FWD_DIRECT) {
- if (iothread && qemuMigrationStopTunnel(iothread, ret < 0) < 0)
- ret = -1;
- }
- VIR_FORCE_CLOSE(fd);
+ if (ret < 0 && iothread)
+ qemuMigrationStopTunnel(iothread, true);
- if (priv->job.completed) {
- qemuDomainJobInfoUpdateTime(priv->job.completed);
- qemuDomainJobInfoUpdateDowntime(priv->job.completed);
- ignore_value(virTimeMillisNow(&priv->job.completed->sent));
- }
+ VIR_FORCE_CLOSE(fd);
if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING)
priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
- cookieFlags |= QEMU_MIGRATION_COOKIE_NETWORK |
- QEMU_MIGRATION_COOKIE_STATS;
-
- if (ret == 0 &&
- (qemuMigrationCookieAddPersistent(mig, &persistDef) < 0 ||
- qemuMigrationBakeCookie(mig, driver, vm, cookieout,
- cookieoutlen, cookieFlags) < 0)) {
- VIR_WARN("Unable to encode migration cookie");
- }
-
virDomainDefFree(persistDef);
qemuMigrationCookieFree(mig);
--
2.14.2
8:56 a.m.
New subject: [libvirt] [PATCH 3/7] qemu: Split cleanup and error code in qemuMigrationRun
Let cleanup only do things common to both failure and success paths and
move error handling code inside the new "error" section.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 79 ++++++++++++++++++++++++-----------------------
1 file changed, 40 insertions(+), 39 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index f0d4f9d98..6956901be 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3657,20 +3657,20 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (persist_xml) {
if (!(persistDef = qemuMigrationPrepareDef(driver, persist_xml,
NULL, NULL)))
- goto cleanup;
+ goto error;
} else {
virDomainDefPtr def = vm->newDef ? vm->newDef : vm->def;
if (!(persistDef = qemuDomainDefCopy(driver, def,
VIR_DOMAIN_XML_SECURE |
VIR_DOMAIN_XML_MIGRATABLE)))
- goto cleanup;
+ goto error;
}
}
mig = qemuMigrationEatCookie(driver, vm, cookiein, cookieinlen,
cookieFlags | QEMU_MIGRATION_COOKIE_GRAPHICS);
if (!mig)
- goto cleanup;
+ goto error;
if (qemuDomainMigrateGraphicsRelocate(driver, vm, mig, graphicsuri) < 0)
VIR_WARN("unable to provide data for graphics client relocation");
@@ -3683,7 +3683,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (qemuMigrationAddTLSObjects(driver, vm, cfg, false,
QEMU_ASYNC_JOB_MIGRATION_OUT,
&tlsAlias, &secAlias, migParams) < 0)
- goto cleanup;
+ goto error;
/* We need to add tls-hostname whenever QEMU itself does not
* connect directly to the destination. */
@@ -3691,17 +3691,17 @@ qemuMigrationRun(virQEMUDriverPtr driver,
spec->destType == MIGRATION_DEST_FD) {
if (VIR_STRDUP(migParams->migrateTLSHostname,
spec->dest.host.name) < 0)
- goto cleanup;
+ goto error;
} else {
/* Be sure there's nothing from a previous migration */
if (VIR_STRDUP(migParams->migrateTLSHostname, "") < 0)
- goto cleanup;
+ goto error;
}
} else {
if (qemuMigrationSetEmptyTLSParams(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT,
migParams) < 0)
- goto cleanup;
+ goto error;
}
if (migrate_flags & (QEMU_MONITOR_MIGRATE_NON_SHARED_DISK |
@@ -3715,7 +3715,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
nmigrate_disks,
migrate_disks,
dconn) < 0) {
- goto cleanup;
+ goto error;
}
} else {
/* Destination doesn't support NBD server.
@@ -3729,37 +3729,37 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (!(flags & VIR_MIGRATE_LIVE) &&
virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING) {
if (qemuMigrationSetOffline(driver, vm) < 0)
- goto cleanup;
+ goto error;
}
if (qemuMigrationSetCompression(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
compression, migParams) < 0)
- goto cleanup;
+ goto error;
if (qemuMigrationSetOption(driver, vm,
QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE,
flags & VIR_MIGRATE_AUTO_CONVERGE,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
- goto cleanup;
+ goto error;
if (qemuMigrationSetOption(driver, vm,
QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL,
flags & VIR_MIGRATE_RDMA_PIN_ALL,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
- goto cleanup;
+ goto error;
if (qemuMigrationSetPostCopy(driver, vm,
flags & VIR_MIGRATE_POSTCOPY,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
- goto cleanup;
+ goto error;
if (qemuMigrationSetParams(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
migParams) < 0)
- goto cleanup;
+ goto error;
if (qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
- goto cleanup;
+ goto error;
if (priv->job.abortJob) {
/* explicitly do this *after* we entered the monitor,
@@ -3770,7 +3770,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
_("canceled by client"));
- goto cleanup;
+ goto error;
}
if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0)
@@ -3817,7 +3817,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (qemuDomainObjExitMonitor(driver, vm) < 0)
ret = -1;
if (ret < 0)
- goto cleanup;
+ goto error;
ret = -1;
/* From this point onwards we *must* call cancel to abort the
@@ -3846,7 +3846,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (rc == -2)
goto cancel;
else if (rc == -1)
- goto cleanup;
+ goto error;
if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
inPostCopy = true;
@@ -3901,28 +3901,10 @@ qemuMigrationRun(virQEMUDriverPtr driver,
ret = 0;
cleanup:
- if (ret < 0 && !orig_err)
- orig_err = virSaveLastError();
-
- /* cancel any outstanding NBD jobs */
- if (ret < 0 && mig && mig->nbd)
- qemuMigrationCancelDriveMirror(driver, vm, false,
- QEMU_ASYNC_JOB_MIGRATION_OUT,
- dconn);
-
VIR_FREE(tlsAlias);
VIR_FREE(secAlias);
virObjectUnref(cfg);
-
- if (ret < 0 && iothread)
- qemuMigrationStopTunnel(iothread, true);
-
VIR_FORCE_CLOSE(fd);
-
- if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
- priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING)
- priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
-
virDomainDefFree(persistDef);
qemuMigrationCookieFree(mig);
@@ -3936,9 +3918,28 @@ qemuMigrationRun(virQEMUDriverPtr driver,
return ret;
+ error:
+ if (!orig_err)
+ orig_err = virSaveLastError();
+
+ /* cancel any outstanding NBD jobs */
+ if (mig && mig->nbd)
+ qemuMigrationCancelDriveMirror(driver, vm, false,
+ QEMU_ASYNC_JOB_MIGRATION_OUT,
+ dconn);
+
+ if (iothread)
+ qemuMigrationStopTunnel(iothread, true);
+
+ if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
+ priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING)
+ priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
+
+ goto cleanup;
+
exit_monitor:
ignore_value(qemuDomainObjExitMonitor(driver, vm));
- goto cleanup;
+ goto error;
cancel:
orig_err = virSaveLastError();
@@ -3950,14 +3951,14 @@ qemuMigrationRun(virQEMUDriverPtr driver,
ignore_value(qemuDomainObjExitMonitor(driver, vm));
}
}
- goto cleanup;
+ goto error;
cancelPostCopy:
priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
if (inPostCopy)
goto cancel;
else
- goto cleanup;
+ goto error;
}
/* Perform migration using QEMU's native migrate support,
--
2.14.2
8:56 a.m.
New subject: [libvirt] [PATCH 4/7] qemu: Unite error handling in qemuMigrationRun
Merge cancel and cancelPostCopy sections with the generic error section,
where we can easily decide whether canceling the ongoing migration is
required.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 61 ++++++++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 33 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 6956901be..cbf255704 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3616,7 +3616,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
unsigned int cookieFlags = 0;
bool abort_on_error = !!(flags & VIR_MIGRATE_ABORT_ON_ERROR);
bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
- bool inPostCopy = false;
+ bool cancel = false;
unsigned int waitFlags;
virDomainDefPtr persistDef = NULL;
char *timestamp;
@@ -3822,10 +3822,11 @@ qemuMigrationRun(virQEMUDriverPtr driver,
/* From this point onwards we *must* call cancel to abort the
* migration on source if anything goes wrong */
+ cancel = true;
if (spec->fwdType != MIGRATION_FWD_DIRECT) {
if (!(iothread = qemuMigrationStartTunnel(spec->fwd.stream, fd)))
- goto cancel;
+ goto error;
/* If we've created a tunnel, then the 'fd' will be closed in the
* qemuMigrationIOFunc as data->sock.
*/
@@ -3843,13 +3844,18 @@ qemuMigrationRun(virQEMUDriverPtr driver,
rc = qemuMigrationWaitForCompletion(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT,
dconn, waitFlags);
- if (rc == -2)
- goto cancel;
- else if (rc == -1)
+ if (rc == -2) {
goto error;
+ } else if (rc == -1) {
+ /* QEMU reported failed migration, nothing to cancel anymore */
+ cancel = false;
+ goto error;
+ }
- if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
- inPostCopy = true;
+ if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
+ /* QEMU finished migration, nothing to cancel anymore */
+ cancel = false;
+ }
/* When migration completed, QEMU will have paused the CPUs for us.
* Wait for the STOP event to be processed or explicitly stop CPUs
@@ -3861,25 +3867,25 @@ qemuMigrationRun(virQEMUDriverPtr driver,
rc = virDomainObjWait(vm);
priv->signalStop = false;
if (rc < 0)
- goto cancelPostCopy;
+ goto error;
}
} else if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_RUNNING &&
qemuMigrationSetOffline(driver, vm) < 0) {
- goto cancelPostCopy;
+ goto error;
}
if (mig && mig->nbd &&
qemuMigrationCancelDriveMirror(driver, vm, true,
QEMU_ASYNC_JOB_MIGRATION_OUT,
dconn) < 0)
- goto cancelPostCopy;
+ goto error;
if (iothread) {
qemuMigrationIOThreadPtr io;
VIR_STEAL_PTR(io, iothread);
if (qemuMigrationStopTunnel(io, false) < 0)
- goto cancelPostCopy;
+ goto error;
}
if (priv->job.completed) {
@@ -3919,8 +3925,15 @@ qemuMigrationRun(virQEMUDriverPtr driver,
return ret;
error:
- if (!orig_err)
- orig_err = virSaveLastError();
+ orig_err = virSaveLastError();
+
+ if (cancel &&
+ virDomainObjIsActive(vm) &&
+ qemuDomainObjEnterMonitorAsync(driver, vm,
+ QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
+ qemuMonitorMigrateCancel(priv->mon);
+ ignore_value(qemuDomainObjExitMonitor(driver, vm));
+ }
/* cancel any outstanding NBD jobs */
if (mig && mig->nbd)
@@ -3932,7 +3945,8 @@ qemuMigrationRun(virQEMUDriverPtr driver,
qemuMigrationStopTunnel(iothread, true);
if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
- priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING)
+ priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
+ priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
goto cleanup;
@@ -3940,25 +3954,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
exit_monitor:
ignore_value(qemuDomainObjExitMonitor(driver, vm));
goto error;
-
- cancel:
- orig_err = virSaveLastError();
-
- if (virDomainObjIsActive(vm)) {
- if (qemuDomainObjEnterMonitorAsync(driver, vm,
- QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
- qemuMonitorMigrateCancel(priv->mon);
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
- }
- }
- goto error;
-
- cancelPostCopy:
- priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
- if (inPostCopy)
- goto cancel;
- else
- goto error;
}
/* Perform migration using QEMU's native migrate support,
--
2.14.2
2:14 a.m.
New subject: [libvirt] [PATCH 4/7] qemu: Unite error handling in qemuMigrationRun
On Thu, Oct 19, 2017 at 15:56:29 +0200, Jiri Denemark wrote:
Merge cancel and cancelPostCopy sections with the generic error
section,
where we can easily decide whether canceling the ongoing migration is
required.
...
@@ -3843,13 +3844,18 @@ qemuMigrationRun(virQEMUDriverPtr driver,
rc = qemuMigrationWaitForCompletion(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT,
dconn, waitFlags);
- if (rc == -2)
- goto cancel;
- else if (rc == -1)
+ if (rc == -2) {
goto error;
+ } else if (rc == -1) {
+ /* QEMU reported failed migration, nothing to cancel anymore */
+ cancel = false;
+ goto error;
+ }
- if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
- inPostCopy = true;
+ if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
+ /* QEMU finished migration, nothing to cancel anymore */
+ cancel = false;
+ }
/* When migration completed, QEMU will have paused the CPUs for us.
* Wait for the STOP event to be processed or explicitly stop CPUs
Please, consider the following small patch squashed in:
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index bc57f757a..626b4e3ee 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3855,11 +3855,6 @@ qemuMigrationRun(virQEMUDriverPtr driver,
goto error;
}
- if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED) {
- /* QEMU finished migration, nothing to cancel anymore */
- cancel = false;
- }
-
/* When migration completed, QEMU will have paused the CPUs for us.
* Wait for the STOP event to be processed or explicitly stop CPUs
* (for old QEMU which does not send events) to release the lock state.
@@ -3931,6 +3926,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
orig_err = virSaveLastError();
if (cancel &&
+ priv->job.current->status != QEMU_DOMAIN_JOB_STATUS_QEMU_COMPLETED
&&
virDomainObjIsActive(vm) &&
qemuDomainObjEnterMonitorAsync(driver, vm,
QEMU_ASYNC_JOB_MIGRATION_OUT) == 0) {
8:56 a.m.
New subject: [libvirt] [PATCH 5/7] qemu: Don't misuse "ret" in qemuMigrationRun
The "ret" variable is used for storing the return value of a function
and should not be used as a temporary variable.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index cbf255704..2d8a634f9 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3788,16 +3788,17 @@ qemuMigrationRun(virQEMUDriverPtr driver,
VIR_FREE(timestamp);
}
+ rc = -1;
switch (spec->destType) {
case MIGRATION_DEST_HOST:
if (STREQ(spec->dest.host.protocol, "rdma") &&
virProcessSetMaxMemLock(vm->pid, vm->def->mem.hard_limit <<
10) < 0) {
goto exit_monitor;
}
- ret = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
- spec->dest.host.protocol,
- spec->dest.host.name,
- spec->dest.host.port);
+ rc = qemuMonitorMigrateToHost(priv->mon, migrate_flags,
+ spec->dest.host.protocol,
+ spec->dest.host.name,
+ spec->dest.host.port);
break;
case MIGRATION_DEST_CONNECT_HOST:
@@ -3809,16 +3810,14 @@ qemuMigrationRun(virQEMUDriverPtr driver,
fd = spec->dest.fd.local;
spec->dest.fd.local = -1;
}
- ret = qemuMonitorMigrateToFd(priv->mon, migrate_flags,
- spec->dest.fd.qemu);
+ rc = qemuMonitorMigrateToFd(priv->mon, migrate_flags,
+ spec->dest.fd.qemu);
VIR_FORCE_CLOSE(spec->dest.fd.qemu);
break;
}
- if (qemuDomainObjExitMonitor(driver, vm) < 0)
- ret = -1;
- if (ret < 0)
+
+ if (qemuDomainObjExitMonitor(driver, vm) < 0 || rc < 0)
goto error;
- ret = -1;
/* From this point onwards we *must* call cancel to abort the
* migration on source if anything goes wrong */
--
2.14.2
8:56 a.m.
New subject: [libvirt] [PATCH 6/7] qemu: Consistently use exit_monitor in qemuMigrationRun
Almost every failure in qemuMigrationRun while we are talking to QEMU
monitor results in a jump to exit_monitor label. The only exception is
removed by this patch.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 2d8a634f9..8a529f9ad 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3765,12 +3765,11 @@ qemuMigrationRun(virQEMUDriverPtr driver,
/* explicitly do this *after* we entered the monitor,
* as this is a critical section so we are guaranteed
* priv->job.abortJob will not change */
- ignore_value(qemuDomainObjExitMonitor(driver, vm));
priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_CANCELED;
virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
_("canceled by client"));
- goto error;
+ goto exit_monitor;
}
if (qemuMonitorSetMigrationSpeed(priv->mon, migrate_speed) < 0)
--
2.14.2
8:56 a.m.
New subject: [libvirt] [PATCH 7/7] qemu: Set correct job status when qemuMigrationRun fails
Instead of enumerating all states which need to be turned into
QEMU_DOMAIN_JOB_STATUS_FAILED (and failing to add all of them), it's
better to mention just the one which needs to be left alone.
Signed-off-by: Jiri Denemark <jdenemar(a)redhat.com>
---
src/qemu/qemu_migration.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 8a529f9ad..f785c308c 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
@@ -3942,9 +3942,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
if (iothread)
qemuMigrationStopTunnel(iothread, true);
- if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
- priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_MIGRATING ||
- priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
+ if (priv->job.current->status != QEMU_DOMAIN_JOB_STATUS_CANCELED)
priv->job.current->status = QEMU_DOMAIN_JOB_STATUS_FAILED;
goto cleanup;
--
2.14.2
8:09 a.m.
On 10/19/2017 09:56 AM, Jiri Denemark wrote:
Mostly refactoring of the horrible mess in qemuMigrationRun.
Jiri Denemark (7):
qemu: Use switch in qemuMigrationCompleted
qemu: Refactor qemuMigrationRun a bit
qemu: Split cleanup and error code in qemuMigrationRun
qemu: Unite error handling in qemuMigrationRun
qemu: Don't misuse "ret" in qemuMigrationRun
qemu: Consistently use exit_monitor in qemuMigrationRun
qemu: Set correct job status when qemuMigrationRun fails
src/qemu/qemu_migration.c | 196 +++++++++++++++++++++++++---------------------
1 file changed, 105 insertions(+), 91 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
(series)
John
2625
days inactive
2626
days old
12 comments
3 participants
participants (3)
-
Jiri Denemark -
John Ferlan -
Marc Hartmayer