On Wed, Dec 28, 2016 at 17:39:15 +0300, Nikolay Shirokovskiy wrote:
> qemuMigrationFetchJobStatus is rather inconvinient. If we poll
> stats for status only then we don't need to update elapsed time.
> Next on some paths we use this function to get stats only we don't
> what to unexpectedly update job status. So the common part
> is only enter/exit which is too little to have a distinct function.
>
> Let's open code getting stats and update elapsed time and status
> where appropriate.
>
> The patch also drops qemuMigrationUpdateJobStatus. It's value
> is only in keeping job status on failures. Now we have option
> not to update status when we want to.
> ---
> src/qemu/qemu_driver.c | 27 ++++++++++++-----
> src/qemu/qemu_migration.c | 74 ++++++++++++++++-------------------------------
> src/qemu/qemu_migration.h | 5 ----
> 3 files changed, 44 insertions(+), 62 deletions(-)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index d1a64d7..d3da18a 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -13062,17 +13062,28 @@ qemuDomainGetJobStatsInternal(virQEMUDriverPtr driver,
> }
> *jobInfo = *info;
>
> - if (jobInfo->status == QEMU_DOMAIN_JOB_STATUS_ACTIVE ||
> - jobInfo->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
> - if (fetch)
> - ret = qemuMigrationFetchJobStatus(driver, vm, QEMU_ASYNC_JOB_NONE,
> - jobInfo);
> - else
> - ret = qemuDomainJobInfoUpdateTime(jobInfo);
> - } else {
> + if (jobInfo->status != QEMU_DOMAIN_JOB_STATUS_ACTIVE &&
> + jobInfo->status != QEMU_DOMAIN_JOB_STATUS_POSTCOPY) {
> ret = 0;
> + goto cleanup;
> }
>
> + if (fetch) {
> + int rv;
> +
> + qemuDomainObjEnterMonitor(driver, vm);
> +
> + rv = qemuMonitorGetMigrationStats(priv->mon, &jobInfo->stats);
> +
> + if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
> + goto cleanup;
> + }
> +
> + if (qemuDomainJobInfoUpdateTime(jobInfo) < 0)
> + goto cleanup;
> +
> + ret = 0;
> +
> cleanup:
> if (fetch)
> qemuDomainObjEndJob(driver, vm);
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 6ddc4bd..5be79df 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -2583,28 +2583,6 @@ qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo)
> }
>
>
> -int
> -qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
> - virDomainObjPtr vm,
> - qemuDomainAsyncJob asyncJob,
> - qemuDomainJobInfoPtr jobInfo)
> -{
> - qemuDomainObjPrivatePtr priv = vm->privateData;
> - int rv;
> -
> - if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> - return -1;
> -
> - rv = qemuMonitorGetMigrationStats(priv->mon, &jobInfo->stats);
> -
> - if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
> - return -1;
> -
> - qemuMigrationUpdateJobType(jobInfo);
> - return qemuDomainJobInfoUpdateTime(jobInfo);
Dropping the two lines above could make sense, but there's no reason to
drop the function completely and copy its contents to three places.
> -}
> -
> -
> static const char *
> qemuMigrationJobName(virDomainObjPtr vm)
> {
> @@ -2624,23 +2602,6 @@ qemuMigrationJobName(virDomainObjPtr vm)
>
>
> static int
> -qemuMigrationUpdateJobStatus(virQEMUDriverPtr driver,
> - virDomainObjPtr vm,
> - qemuDomainAsyncJob asyncJob)
> -{
> - qemuDomainObjPrivatePtr priv = vm->privateData;
> - qemuDomainJobInfoPtr jobInfo = priv->job.current;
> - qemuDomainJobInfo newInfo = *jobInfo;
> -
> - if (qemuMigrationFetchJobStatus(driver, vm, asyncJob, &newInfo) < 0)
> - return -1;
> -
> - *jobInfo = newInfo;
> - return 0;
> -}
The goal of this function is to avoid touching priv->job.current in case
of failure. Since qemuMonitorGetMigrationStats memsets
qemuDomainJobInfo, we would lose its original contents, which is
definitely not something we want.
I'm not against refactoring this code, but we need to keep the current
behavior of qemuMigrationUpdateJobStatus in some way.
NACK to this version.
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
