Re: [libvirt] [PATCH v3 REBASE 06/16] qemu: refactor fetching migration stats
On 29.08.2017 16:45, Jiri Denemark wrote:
On Thu, Aug 24, 2017 at 09:56:43 +0300, Nikolay Shirokovskiy wrote:
> qemuMigrationFetchJobStatus is rather inconvinient. Some of its
> callers don't need status to be updated, some don't need to update
> elapsed time right away. So let's update status or elapsed time
> in callers instead.
>
> In qemuMigrationConfirmPhase we should fetch stats with copy
> flag set as stats variable refers to domain object not the stack.
>
> This patch drops updating job status on getting job stats by
> client. This way we will not provide status 'completed' while
> it is not yet updated by migration routine.
...
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index cc42f7a..dec0a08 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -1376,24 +1376,28 @@ qemuMigrationUpdateJobType(qemuDomainJobInfoPtr jobInfo)
>
>
> int
> -qemuMigrationFetchJobStatus(virQEMUDriverPtr driver,
> - virDomainObjPtr vm,
> - qemuDomainAsyncJob asyncJob,
> - qemuDomainJobInfoPtr jobInfo)
> +qemuMigrationFetchMigrationStats(virQEMUDriverPtr driver,
The name contains "Migration" twice. How about qemuMigrationFetchStats
or qemuMigrationFetchJobStats?
Both are good. I like qemuMigrationFetchStats.
> + virDomainObjPtr vm,
> + qemuDomainAsyncJob asyncJob,
> + qemuMonitorMigrationStatsPtr stats,
It looks like all callers are always passing something like
&jobInfo->stats so keeping qemuDomainJobInfoPtr jobInfo argument could
make the callers a bit simpler.
Ok.
> + bool copy)
I'd just drop the "copy" parameter completely and let the function
always fetch stats in a local variable and then copy its content into
the "stats" argument. I.e., make it always work as if copy == true.
Ok.
> {
> qemuDomainObjPrivatePtr priv = vm->privateData;
> + qemuMonitorMigrationStats statsCopy;
> int rv;
>
> if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
> return -1;
>
> - rv = qemuMonitorGetMigrationStats(priv->mon, &jobInfo->stats);
> + rv = qemuMonitorGetMigrationStats(priv->mon, copy ? &statsCopy : stats);
>
> if (qemuDomainObjExitMonitor(driver, vm) < 0 || rv < 0)
> return -1;
>
> - qemuMigrationUpdateJobType(jobInfo);
> - return qemuDomainJobInfoUpdateTime(jobInfo);
> + if (copy)
> + *stats = statsCopy;
> +
> + return 0;
> }
>
>
...
> @@ -1442,11 +1429,12 @@ qemuMigrationCheckJobStatus(virQEMUDriverPtr driver,
>
> bool events = virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_MIGRATION_EVENT);
>
> - if (events)
> - qemuMigrationUpdateJobType(jobInfo);
> - else if (qemuMigrationUpdateJobStatus(driver, vm, asyncJob) < 0)
> + if (!events && qemuMigrationFetchMigrationStats(driver, vm, asyncJob,
Break the line after &&, please.
> + &jobInfo->stats, true)
< 0)
> return -1;
>
> + qemuMigrationUpdateJobType(jobInfo);
> +
> switch (jobInfo->status) {
> case QEMU_DOMAIN_JOB_STATUS_NONE:
> virReportError(VIR_ERR_OPERATION_FAILED, _("%s: %s"),
...
Jirka