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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
John
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
