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

Re: [libvirt] [PATCH v3 REBASE 03/16] qemu: introduce QEMU_DOMAIN_JOB_STATUS_POSTCOPY




On 28.08.2017 17:55, Jiri Denemark wrote:
> On Thu, Aug 24, 2017 at 09:56:40 +0300, Nikolay Shirokovskiy wrote:
>> Current code consults job.current->stats.status to check for postcopy
>> state. First it is more correct to check for both job.current->status
>> and job.current->stats.status.code because on some paths on failures
>> we change only the former. Second if qemu supports migration events
>> then stats can change unexpectedly.
> 
> I'm not sure I understand what you're trying to say. Could you explain
> this a bit more?

This place looks dangerous to me:

@@ -3836,7 +3840,7 @@ qemuMigrationRun(virQEMUDriverPtr driver,
     else if (rc == -1)
         goto cleanup;
 
-    if (priv->job.current->stats.status == QEMU_MONITOR_MIGRATION_STATUS_POSTCOPY)
+    if (priv->job.current->status == QEMU_DOMAIN_JOB_STATUS_POSTCOPY)
         inPostCopy = true;

It makes decisions based on current state of migration and consult stats.status for
this purpose. Such a check can be mistaken because in the code above this hunk if error occurs we change only current->status
value. Also after patch 6 we can update stats.status during fetching migrations
stats but not change current->status.

However in a more detail analysis it looks like this code do not have undesired
effects and only look dangerous (at least to me). So I guess I'd better say it is easier
to reason about if we have postcopy as one of the status states.

> 
>> Let's introduce QEMU_DOMAIN_JOB_STATUS_POSTCOPY state for job.current->status.
>>
>> This patch removes all state checking usage of stats except for
>> qemuDomainGetJobStatsInternal. This place will be handled separately.
>> ---
>>  src/qemu/qemu_domain.c    |  1 +
>>  src/qemu/qemu_domain.h    |  1 +
>>  src/qemu/qemu_driver.c    |  5 +++--
>>  src/qemu/qemu_migration.c | 18 +++++++++++-------
>>  src/qemu/qemu_process.c   |  4 ++--
>>  5 files changed, 18 insertions(+), 11 deletions(-)
> 
> ACK
> 
> Jirka
> 


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