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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
