On Thu, Sep 01, 2022 at 14:47:40 +0200, Jiri Denemark wrote:
> We will need a little bit more code around qemuMonitorMigrateCancel to
> make sure it works as expected. The new qemuMigrationSrcCancel helper
> will avoid repeating the code in several places.
>
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
> src/qemu/qemu_driver.c | 9 +--------
> src/qemu/qemu_migration.c | 37 +++++++++++++++++++++++--------------
> src/qemu/qemu_migration.h | 4 ++++
> src/qemu/qemu_process.c | 5 +----
> 4 files changed, 29 insertions(+), 26 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 707f4cc1bb..a86efc769a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -12808,17 +12808,10 @@ qemuDomainGetJobStats(virDomainPtr dom,
> static int
> qemuDomainAbortJobMigration(virDomainObj *vm)
> {
> - qemuDomainObjPrivate *priv = vm->privateData;
> - int ret;
> -
> VIR_DEBUG("Cancelling migration job at client request");
>
> qemuDomainObjAbortAsyncJob(vm);
> - qemuDomainObjEnterMonitor(vm);
> - ret = qemuMonitorMigrateCancel(priv->mon);
So this caller cared about the return value of
'qemuMonitorMigrateCancel' ...
> - qemuDomainObjExitMonitor(vm);
> -
> - return ret;
> + return qemuMigrationSrcCancel(vm, VIR_ASYNC_JOB_NONE);
> }
>
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 67d83ca743..5845dfdb9c 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -4611,6 +4611,24 @@ qemuMigrationSrcStart(virDomainObj *vm,
> }
>
>
> +int
> +qemuMigrationSrcCancel(virDomainObj *vm,
> + virDomainAsyncJob asyncJob)
> +{
> + qemuDomainObjPrivate *priv = vm->privateData;
> +
> + VIR_DEBUG("Cancelling outgoing migration of domain %s", vm->def->name);
> +
> + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
> + return -1;
> +
> + qemuMonitorMigrateCancel(priv->mon);
... but here you don't propagate it out. Instead the only possibility
for this function to fail is from 'qemuDomainObjEnterMonitorAsync', but
any caller that pases non-NONE asyncjob doesn't care about the return
value at all.
> + qemuDomainObjExitMonitor(vm);
> +
> + return 0;
> +}
> +
> +
Also consider adding a comment too.
With the above bug fixed:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
