The cleanup function can in many cases needs cleanup on its own.
The major thing we want to do here is not referencing to_dst_file when
without the file mutex. When at it, touch things elsewhere too to make it
look slightly better in general.
One thing to mention is, migration_thread has its own "running" boolean, so
it doesn't need to rely on to_dst_file being non-NULL. Multifd has a
dependency so it needs to be skipped if to_dst_file is not yet set; add a
richer comment for such reason.
Resolves: Coverity CID 1527402
Reported-by: Peter Maydell <peter.mayd...@linaro.org>
Signed-off-by: Peter Xu <pet...@redhat.com>
---
migration/migration.c | 32 +++++++++++++++++++-------------
1 file changed, 19 insertions(+), 13 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index ae2be31557..a10fae1aee 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1405,6 +1405,9 @@ void migrate_set_state(MigrationStatus *state,
MigrationStatus old_state,
static void migrate_fd_cleanup(MigrationState *s)
{
MigrationEventType type;
+ QEMUFile *tmp = NULL;
+
+ trace_migrate_fd_cleanup();
g_free(s->hostname);
s->hostname = NULL;
@@ -1415,26 +1418,29 @@ static void migrate_fd_cleanup(MigrationState *s)
close_return_path_on_source(s);
- if (s->to_dst_file) {
- QEMUFile *tmp;
-
- trace_migrate_fd_cleanup();
+ if (s->migration_thread_running) {
bql_unlock();
- if (s->migration_thread_running) {
- qemu_thread_join(&s->thread);
- s->migration_thread_running = false;
- }
+ qemu_thread_join(&s->thread);
+ s->migration_thread_running = false;
bql_lock();
+ }
- multifd_send_shutdown();
- qemu_mutex_lock(&s->qemu_file_lock);
+ WITH_QEMU_LOCK_GUARD(&s->qemu_file_lock) {
+ /*
+ * Close the file handle without the lock to make sure the critical
+ * section won't block for long.
+ */
tmp = s->to_dst_file;
s->to_dst_file = NULL;
- qemu_mutex_unlock(&s->qemu_file_lock);
+ }
+
+ if (tmp) {
/*
- * Close the file handle without the lock to make sure the
- * critical section won't block for long.
+ * We only need to shutdown multifd if tmp!=NULL, because if
+ * tmp==NULL, it means the main channel isn't established, while
+ * multifd is only setup after that (in migration_thread()).
*/
+ multifd_send_shutdown();
migration_ioc_unregister_yank_from_file(tmp);
qemu_fclose(tmp);
}
--
2.45.0
