[PATCH 0/5] qemu: Migration code (ternary operator) cleanups
I've noticed few things that could be improved during review of Jirka's post copy migration changes but didn't want to interfer with the series at that point. Peter Krempa (5): qemuMigrationDstFinishFresh: Avoid multi-line ternary operator in function call qemuMigrationDstPersist: Avoid multi-line ternary operator in function call qemu: migration: Overwrite 'dname' only when NULL qemuMigrationSrcIOFunc: Avoid unnecessary string construction qemu: monitor: Split up enum strings definitions src/qemu/qemu_migration.c | 37 +++++++++++++++++++++++-------------- src/qemu/qemu_monitor.c | 35 ++++++++++++++++++++++++++--------- 2 files changed, 49 insertions(+), 23 deletions(-) -- 2.36.1
Rewrite the code using a temporary variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20dc91f1ce..800f66349d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -6601,9 +6601,14 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver, *inPostCopy = true; if (!(flags & VIR_MIGRATE_PAUSED)) { - if (qemuProcessStartCPUs(driver, vm, - *inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY - : VIR_DOMAIN_RUNNING_MIGRATED, + virDomainRunningReason runningReason; + + if (inPostCopy) + runningReason = VIR_DOMAIN_RUNNING_POSTCOPY; + else + runningReason = VIR_DOMAIN_RUNNING_MIGRATED; + + if (qemuProcessStartCPUs(driver, vm, runningReason, VIR_ASYNC_JOB_MIGRATION_IN) < 0) { if (virGetLastErrorCode() == VIR_ERR_OK) virReportError(VIR_ERR_INTERNAL_ERROR, -- 2.36.1
On 7/25/22 14:45, Peter Krempa wrote:
Rewrite the code using a temporary variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20dc91f1ce..800f66349d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -6601,9 +6601,14 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver, *inPostCopy = true;
if (!(flags & VIR_MIGRATE_PAUSED)) { - if (qemuProcessStartCPUs(driver, vm, - *inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY - : VIR_DOMAIN_RUNNING_MIGRATED, + virDomainRunningReason runningReason; + + if (inPostCopy)
This needs to dereference the variable, just like the original did. And what is your opinion on initializing the newly introduced variable to _MIGRATED and then having this if() to overwrite it to _POSTCOPY? virDomainRunningReason runningReason = VIR_DOMAIN_RUNNING_MIGRATED; if (*inPostCopy) runningReason = VIR_DOMAIN_RUNNING_POSTCOPY; Michal
On Mon, Jul 25, 2022 at 15:51:28 +0200, Michal Prívozník wrote:
On 7/25/22 14:45, Peter Krempa wrote:
Rewrite the code using a temporary variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20dc91f1ce..800f66349d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -6601,9 +6601,14 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver, *inPostCopy = true;
if (!(flags & VIR_MIGRATE_PAUSED)) { - if (qemuProcessStartCPUs(driver, vm, - *inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY - : VIR_DOMAIN_RUNNING_MIGRATED, + virDomainRunningReason runningReason; + + if (inPostCopy)
This needs to dereference the variable, just like the original did.
Oops, indeed.
And what is your opinion on initializing the newly introduced variable to _MIGRATED and then having this if() to overwrite it to _POSTCOPY?
virDomainRunningReason runningReason = VIR_DOMAIN_RUNNING_MIGRATED;
if (*inPostCopy) runningReason = VIR_DOMAIN_RUNNING_POSTCOPY;
I actually had it like this at first, but decided to change it after the same change in patch 2/5. I wanted to have both assignments close to the usage place to make it obvious what the value is. In this case the initialization assignment is close enough, though.
On Mon, Jul 25, 2022 at 16:19:38 +0200, Peter Krempa wrote:
On Mon, Jul 25, 2022 at 15:51:28 +0200, Michal Prívozník wrote:
On 7/25/22 14:45, Peter Krempa wrote:
Rewrite the code using a temporary variable.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 20dc91f1ce..800f66349d 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -6601,9 +6601,14 @@ qemuMigrationDstFinishFresh(virQEMUDriver *driver, *inPostCopy = true;
if (!(flags & VIR_MIGRATE_PAUSED)) { - if (qemuProcessStartCPUs(driver, vm, - *inPostCopy ? VIR_DOMAIN_RUNNING_POSTCOPY - : VIR_DOMAIN_RUNNING_MIGRATED, + virDomainRunningReason runningReason; + + if (inPostCopy)
This needs to dereference the variable, just like the original did.
Oops, indeed.
And what is your opinion on initializing the newly introduced variable to _MIGRATED and then having this if() to overwrite it to _POSTCOPY?
virDomainRunningReason runningReason = VIR_DOMAIN_RUNNING_MIGRATED;
if (*inPostCopy) runningReason = VIR_DOMAIN_RUNNING_POSTCOPY;
I actually had it like this at first, but decided to change it after the same change in patch 2/5. I wanted to have both assignments close to the usage place to make it obvious what the value is.
In this case the initialization assignment is close enough, though.
Looking at the patches again, I in fact forgot to do that change to 2/5, so .... I'll just go with this suggestion :D
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 800f66349d..a14b774a22 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -6393,6 +6393,7 @@ qemuMigrationDstPersist(virQEMUDriver *driver, g_autoptr(virDomainDef) oldDef = NULL; unsigned int oldPersist = vm->persistent; virObjectEvent *event; + virDomainEventDefinedDetailType eventDetail = VIR_DOMAIN_EVENT_DEFINED_UPDATED; vm->persistent = 1; oldDef = vm->newDef; @@ -6402,18 +6403,19 @@ qemuMigrationDstPersist(virQEMUDriver *driver, priv->qemuCaps))) goto error; - if (!oldPersist && qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0) - goto error; + if (!oldPersist) { + eventDetail = VIR_DOMAIN_EVENT_DEFINED_ADDED; + + if (qemuDomainNamePathsCleanup(cfg, vmdef->name, false) < 0) + goto error; + } if (virDomainDefSave(vmdef, driver->xmlopt, cfg->configDir) < 0 && !ignoreSaveError) goto error; - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_DEFINED, - oldPersist ? - VIR_DOMAIN_EVENT_DEFINED_UPDATED : - VIR_DOMAIN_EVENT_DEFINED_ADDED); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_DEFINED, + eventDetail); virObjectEventStateQueue(driver->domainEventState, event); return 0; -- 2.36.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index a14b774a22..0d696e1d46 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -5422,7 +5422,8 @@ qemuMigrationSrcPerformPeer2Peer2(virQEMUDriver *driver, * status code from the sender to the destination host, * so it can do any cleanup if the migration failed. */ - dname = dname ? dname : vm->def->name; + if (!dname) + dname = vm->def->name; VIR_DEBUG("Finish2 %p ret=%d", dconn, ret); qemuDomainObjEnterRemote(vm); ddomain = dconn->driver->domainMigrateFinish2 @@ -5707,7 +5708,8 @@ qemuMigrationSrcPerformPeer2Peer3(virQEMUDriver *driver, goto cleanup; } } else { - dname = dname ? dname : vm->def->name; + if (!dname) + dname = vm->def->name; qemuDomainObjEnterRemote(vm); ddomain = dconn->driver->domainMigrateFinish3 (dconn, dname, cookiein, cookieinlen, &cookieout, &cookieoutlen, -- 2.36.1
Use full strings for better greppability. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_migration.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index 0d696e1d46..f2ccbe11b9 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -4267,11 +4267,11 @@ static void qemuMigrationSrcIOFunc(void *arg) goto abrt; } - VIR_DEBUG("Migration tunnel was asked to %s", - stop ? "abort" : "finish"); if (stop) { + VIR_DEBUG("Migration tunnel was asked to stop"); goto abrt; } else { + VIR_DEBUG("Migration tunnel was asked to finish"); timeout = 0; } } -- 2.36.1
The VIR_ENUM_IMPL macros directly above them list one string per line. Use the same also for qemuMonitorMigrationStatus and qemuMonitorVMStatus. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_monitor.c | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 9687746703..6ebdeb46f3 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -144,20 +144,37 @@ VIR_ENUM_IMPL(qemuMonitorCPUProperty, VIR_ENUM_IMPL(qemuMonitorMigrationStatus, QEMU_MONITOR_MIGRATION_STATUS_LAST, - "inactive", "setup", - "active", "pre-switchover", - "device", "postcopy-active", - "postcopy-paused", "postcopy-recover", - "completed", "failed", - "cancelling", "cancelled", + "inactive", + "setup", + "active", + "pre-switchover", + "device", + "postcopy-active", + "postcopy-paused", + "postcopy-recover", + "completed", + "failed", + "cancelling", + "cancelled", "wait-unplug", ); VIR_ENUM_IMPL(qemuMonitorVMStatus, QEMU_MONITOR_VM_STATUS_LAST, - "debug", "inmigrate", "internal-error", "io-error", "paused", - "postmigrate", "prelaunch", "finish-migrate", "restore-vm", - "running", "save-vm", "shutdown", "watchdog", "guest-panicked", + "debug", + "inmigrate", + "internal-error", + "io-error", + "paused", + "postmigrate", + "prelaunch", + "finish-migrate", + "restore-vm", + "running", + "save-vm", + "shutdown", + "watchdog", + "guest-panicked", ); typedef enum { -- 2.36.1
On 7/25/22 14:45, Peter Krempa wrote:
I've noticed few things that could be improved during review of Jirka's post copy migration changes but didn't want to interfer with the series at that point.
Peter Krempa (5): qemuMigrationDstFinishFresh: Avoid multi-line ternary operator in function call qemuMigrationDstPersist: Avoid multi-line ternary operator in function call qemu: migration: Overwrite 'dname' only when NULL qemuMigrationSrcIOFunc: Avoid unnecessary string construction qemu: monitor: Split up enum strings definitions
src/qemu/qemu_migration.c | 37 +++++++++++++++++++++++-------------- src/qemu/qemu_monitor.c | 35 ++++++++++++++++++++++++++--------- 2 files changed, 49 insertions(+), 23 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> But see my comment to 1/5 before pushing. There's one typo that needs fixing. Michal
participants (2)
-
Michal Prívozník -
Peter Krempa