| Message ID | 20240919163042.116767-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Cleanup migrate_fd_cleanup() on accessing to_dst_file | expand |
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>
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.
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); }
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(-)