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