[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v3 REBASE 06/16] qemu: refactor fetching migration stats



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?

> +                                 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.

> +                                 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.

>  {
>      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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]