7:45 a.m.
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
7:45 a.m.
New subject: [PATCH 1/5] qemuMigrationDstFinishFresh: Avoid multi-line ternary operator in function call
Rewrite the code using a temporary variable.
Signed-off-by: Peter Krempa <pkrempa(a)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
8:51 a.m.
New subject: [PATCH 1/5] qemuMigrationDstFinishFresh: Avoid multi-line ternary operator in function call
On 7/25/22 14:45, Peter Krempa wrote:
Rewrite the code using a temporary variable.
Signed-off-by: Peter Krempa <pkrempa(a)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
9:19 a.m.
New subject: [PATCH 1/5] qemuMigrationDstFinishFresh: Avoid multi-line ternary operator in function call
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(a)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.
9:21 a.m.
New subject: [PATCH 1/5] qemuMigrationDstFinishFresh: Avoid multi-line ternary operator in function call
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(a)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
7:45 a.m.
New subject: [PATCH 2/5] qemuMigrationDstPersist: Avoid multi-line ternary operator in function call
Signed-off-by: Peter Krempa <pkrempa(a)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
7:45 a.m.
New subject: [PATCH 3/5] qemu: migration: Overwrite 'dname' only when NULL
Signed-off-by: Peter Krempa <pkrempa(a)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
7:45 a.m.
New subject: [PATCH 4/5] qemuMigrationSrcIOFunc: Avoid unnecessary string construction
Use full strings for better greppability.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:45 a.m.
New subject: [PATCH 5/5] qemu: monitor: Split up enum strings definitions
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(a)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
8:51 a.m.
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(a)redhat.com>
But see my comment to 1/5 before pushing. There's one typo that needs
fixing.
Michal
851
days inactive
851
days old
9 comments
2 participants
participants (2)
-
Michal Prívozník -
Peter Krempa