[PATCH] migration: Cleanup migrate_fd_cleanup() on accessing to_dst_file

[PATCH] migration: Cleanup migrate_fd_cleanup() on accessing to_dst_file

[Peter Xu]

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.maydell@linaro.org>
Signed-off-by: Peter Xu <peterx@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

Re: [PATCH] migration: Cleanup migrate_fd_cleanup() on accessing to_dst_file

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> 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.maydell@linaro.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH] migration: Cleanup migrate_fd_cleanup() on accessing to_dst_file

[Peter Xu]

On Thu, Sep 19, 2024 at 12:30:42PM -0400, Peter Xu wrote:
> 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.maydell@linaro.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>

queued.

-- 
Peter Xu