5:31 p.m.
On 02/16/2018 06:22 AM, Daniel P. Berrangé wrote:
It is very difficult while reading the migration code trying to
understand whether a particular function is being called on the src side
or the dst side, or either. Putting "Src" or "Dst" in the method
names will
make this much more obvious. "Any" is used in a few helpers which can be
called from both sides.
This is quite an understatement!!! Maybe now I can throw away those
pieces of paper where I wrote down which side was which B-). I almost
wonder if the next step is "qemu_migration_{src|dst|any}.{c|h}.
Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com>
---
src/qemu/qemu_driver.c | 194 +++---
src/qemu/qemu_migration.c | 1440 +++++++++++++++++++++++----------------------
src/qemu/qemu_migration.h | 267 +++++----
src/qemu/qemu_process.c | 20 +-
tests/qemuxml2argvtest.c | 4 +-
5 files changed, 972 insertions(+), 953 deletions(-)
Hopefully there's not some poor soul working through migration patches
that's going to have a fun merge. The names look reasonable to me.
Hopefully Jirka also can take a peek.
Is there a way to ensure that future qemuMigration API's are prefixed
with Dst, Src, or Any (disregarding ParamsXXX and JobXXX) for make
syntax-check type purposes?
[...]
diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
index 29247d6a39..7f981f8f2f 100644
--- a/src/qemu/qemu_migration.c
+++ b/src/qemu/qemu_migration.c
[...]
/* Returns true if the domain was resumed, false otherwise */
static bool
-qemuMigrationRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
+qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
int reason;
@@ -320,9 +320,9 @@ qemuMigrationRestoreDomainState(virQEMUDriverPtr driver,
virDomainObjPtr vm)
Should the qemuMigrateDisk here change to qemuMigrateAnyCheckDisk ??
[...]
Function comments for qemuMigrationSrcDriveMirror still list
qemuMigrationDriveMirror
* simultaneously to both source and destination. On success,
* update @migrate_flags so we don't tell 'migrate' command
* to do the very same operation. On failure, the caller is
- * expected to call qemuMigrationCancelDriveMirror to stop all
+ * expected to call qemuMigrationSrcCancelDriveMirror to stop all
* running mirrors.
*
* Returns 0 on success (@migrate_flags updated),
* -1 otherwise.
*/
static int
-qemuMigrationDriveMirror(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- qemuMigrationCookiePtr mig,
- const char *host,
- unsigned long speed,
- unsigned int *migrate_flags,
- size_t nmigrate_disks,
- const char **migrate_disks,
- virConnectPtr dconn)
+qemuMigrationSrcDriveMirror(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuMigrationCookiePtr mig,
+ const char *host,
+ unsigned long speed,
+ unsigned int *migrate_flags,
+ size_t nmigrate_disks,
+ const char **migrate_disks,
+ virConnectPtr dconn)
[...]
static int
-qemuMigrationSetPostCopy(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- bool state,
- qemuDomainAsyncJob job)
+qemuMigrationAnySetPostCopy(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ bool state,
+ qemuDomainAsyncJob job)
{
qemuDomainObjPrivatePtr priv = vm->privateData;
- if (qemuMigrationSetOption(driver, vm,
- QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY,
- state, job) < 0)
+ if (qemuMigrationAnySetOption(driver, vm,
+ QEMU_MONITOR_MIGRATION_CAPS_POSTCOPY,
+ state, job) < 0)
return -1;
priv->job.postcopyEnabled = state;
The qemuMigrationWaitForSpice probably should change to
qemuMigrationSrcWaitForSpice since it's called from
qemuMigrationSrcConfirmPhase.
[...]
static const char *
-qemuMigrationJobName(virDomainObjPtr vm)
+qemuMigrationAnyJobName(virDomainObjPtr vm)
{
Should this change back without the "Any" (based on qemu_migration.h
comment)
static int
-qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- qemuDomainAsyncJob asyncJob)
+qemuMigrationAnyCheckJobStatus(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob)
Similar, but perhaps qemuMigrationJobCheckStatus ?
[...]
int
-qemuMigrationCheckIncoming(virQEMUCapsPtr qemuCaps,
- const char *migrateFrom)
+qemuMigrationDstCheckIncoming(virQEMUCapsPtr qemuCaps,
+ const char *migrateFrom)
Dst and Incoming almost seem superfluous, this is more like
qemuMigrationDstCheckProtocol (since we're already changing names
elsewhere).
[...]
char *
-qemuMigrationIncomingURI(const char *migrateFrom,
- int migrateFd)
+qemuMigrationDstIncomingURI(const char *migrateFrom,
+ int migrateFd)
qemuMigrationDstGetURI ? IDC, the current name is fine, just a suggestion.
[...]
int
-qemuMigrationRunIncoming(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- const char *uri,
- qemuDomainAsyncJob asyncJob)
+qemuMigrationDstRunIncoming(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ const char *uri,
+ qemuDomainAsyncJob asyncJob)
qemuMigrationDstRun to match SrcRun ?? Your call.
[...]
static qemuProcessIncomingDefPtr
-qemuMigrationPrepareIncoming(virDomainObjPtr vm,
- bool tunnel,
- const char *protocol,
- const char *listenAddress,
- unsigned short port,
- int fd)
+qemuMigrationDstPrepareIncoming(virDomainObjPtr vm,
+ bool tunnel,
+ const char *protocol,
+ const char *listenAddress,
+ unsigned short port,
+ int fd)
qemuMigrationDstPrepare (again Incoming just seems unnecessary w/ Dst)
[...]
-/* qemuMigrationSetEmptyTLSParams
+/* qemuMigrationAnySetEmptyTLSParams
* @driver: pointer to qemu driver
* @vm: domain object
* @asyncJob: migration job to join
@@ -2433,14 +2435,14 @@ qemuMigrationParamsFree(qemuMonitorMigrationParamsPtr
*migParams)
* Returns 0 on success, -1 on failure
*/
static int
-qemuMigrationSetEmptyTLSParams(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- qemuDomainAsyncJob asyncJob,
- qemuMonitorMigrationParamsPtr migParams)
+qemuMigrationAnySetEmptyTLSParams(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob,
+ qemuMonitorMigrationParamsPtr migParams)
Perhaps change to qemuMigrationParamsSetEmptyTLS (avoiding the Any w/
any of the qemuMigration*Params related helpers)
static int
-qemuMigrationSetParams(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- qemuDomainAsyncJob job,
- qemuMonitorMigrationParamsPtr migParams)
+qemuMigrationAnySetParams(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob job,
+ qemuMonitorMigrationParamsPtr migParams)
qemuMigrationParamsSet ??
[...]
-/* qemuMigrationResetTLS
+/* qemuMigrationAnyResetTLS
* @driver: pointer to qemu driver
* @vm: domain object
* @asyncJob: migration job to join
@@ -2536,9 +2538,9 @@ qemuMigrationSetParams(virQEMUDriverPtr driver,
* Returns 0 on success, -1 on failure
*/
static int
-qemuMigrationResetTLS(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- qemuDomainAsyncJob asyncJob)
+qemuMigrationAnyResetTLS(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ qemuDomainAsyncJob asyncJob)
qemuMigrationParamsResetTLS ??
[...]
int
-qemuMigrationConfirm(virQEMUDriverPtr driver,
- virDomainObjPtr vm,
- const char *cookiein,
- int cookieinlen,
- unsigned int flags,
- int cancelled)
+qemuMigrationSrcConfirm(virQEMUDriverPtr driver,
+ virDomainObjPtr vm,
+ const char *cookiein,
+ int cookieinlen,
+ unsigned int flags,
+ int cancelled)
{
qemuMigrationJobPhase phase;
virQEMUDriverConfigPtr cfg = NULL;
@@ -3365,9 +3367,9 @@ qemuMigrationConfirm(virQEMUDriverPtr driver,
qemuMigrationJobStartPhase(driver, vm, phase);
virCloseCallbacksUnset(driver->closeCallbacks, vm,
- qemuMigrationCleanup);
+ qemuMigrationSrcCleanup);
- ret = qemuMigrationConfirmPhase(driver, vm,
+ ret = qemuMigrationSrcConfirmPhase(driver, vm,
cookiein, cookieinlen,
flags, cancelled);
Indention adjustment needed
[...]
diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
index 234f1eb858..f769f7417d 100644
--- a/src/qemu/qemu_migration.h
+++ b/src/qemu/qemu_migration.h
[...]
@@ -130,170 +143,170 @@
qemuMigrationParamsFree(qemuMonitorMigrationParamsPtr *migParams);
qemuMonitorMigrationParamsPtr
qemuMigrationParams(virTypedParameterPtr params,
- int nparams,
- unsigned long flags);
+ int nparams,
+ unsigned long flags);
Looks like a couple of strays from a previous edit to change MigrationParams
[...]
void
-qemuMigrationPostcopyFailed(virQEMUDriverPtr driver,
+qemuMigrationAnyPostcopyFailed(virQEMUDriverPtr driver,
virDomainObjPtr vm);
This one needs the indention
[...]
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John