On Tue, Aug 18, 2020 at 13:26:35 +0300, Nikolay Shirokovskiy wrote:
> qemuMigrationSrcCleanup uses qemuDomainObjDiscardAsyncJob currently. But
> discard does not reduce jobs_queued counter so it leaks. Also discard does not
> notify other threads that job condition is available. Discard does reset nested
> job but nested job is not possible in this conditions.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
> ---
> src/qemu/qemu_migration.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 142faa2..301475a 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2026,7 +2026,6 @@ qemuMigrationSrcCleanup(virDomainObjPtr vm,
> switch ((qemuMigrationJobPhase) priv->job.phase) {
> case QEMU_MIGRATION_PHASE_BEGIN3:
> /* just forget we were about to migrate */
> - qemuDomainObjDiscardAsyncJob(driver, vm);
> break;
>
> case QEMU_MIGRATION_PHASE_PERFORM3_DONE:
> @@ -2036,7 +2035,6 @@ qemuMigrationSrcCleanup(virDomainObjPtr vm,
> qemuMigrationParamsReset(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT,
> jobPriv->migParams, priv->job.apiFlags);
> /* clear the job and let higher levels decide what to do */
> - qemuDomainObjDiscardAsyncJob(driver, vm);
> break;
>
> case QEMU_MIGRATION_PHASE_PERFORM3:
> @@ -2053,8 +2051,9 @@ qemuMigrationSrcCleanup(virDomainObjPtr vm,
> case QEMU_MIGRATION_PHASE_NONE:
> case QEMU_MIGRATION_PHASE_LAST:
> /* unreachable */
> - ;
> + return;
> }
> + qemuMigrationJobFinish(driver, vm);
> }
I was thinking whether we could somehow merge the two APIs, but
apparently we need both as some callers should not do the actions in
which these two APIs differ.
Anyway, this patch is good, but I'd prefer the qemuMigrationJobFinish
call to stay in each switch branch to make the code executed for each
phase immediately visible. And it's just one line so I don't see a
reason for moving it.
With the suggested change:
Reviewed-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
