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

[Nikolay Shirokovskiy]

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
>