| Message ID | 20230811150836.2895-10-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | Fix segfault on migration return path | expand |
On Fri, Aug 11, 2023 at 12:08:35PM -0300, Fabiano Rosas wrote: > When doing cleanup, we currently close() some of the shared migration > files and shutdown() + close() others. Be consistent by always calling > shutdown() before close(). > > Do this only for the source files for now because the source runs > multiple threads which could cause races between the two calls. Having > them together allows us to move them to a centralized place under the > protection of a lock the next patch. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> Logically I think we should only need shutdown() when we don't want to close immediately, or can't for some reason.. Maybe instead of adding shutdown()s, we can remove some? > --- > migration/migration.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/migration/migration.c b/migration/migration.c > index 7fec57ad7f..4df5ca25c1 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -1175,6 +1175,7 @@ static void migrate_fd_cleanup(MigrationState *s) > * critical section won't block for long. > */ > migration_ioc_unregister_yank_from_file(tmp); > + qemu_file_shutdown(tmp); > qemu_fclose(tmp); > } > > @@ -1844,6 +1845,7 @@ static void migration_release_dst_files(MigrationState *ms) > ms->postcopy_qemufile_src = NULL; > } > > + qemu_file_shutdown(file); > qemu_fclose(file); > } > > -- > 2.35.3 >
Peter Xu <peterx@redhat.com> writes: > On Fri, Aug 11, 2023 at 12:08:35PM -0300, Fabiano Rosas wrote: >> When doing cleanup, we currently close() some of the shared migration >> files and shutdown() + close() others. Be consistent by always calling >> shutdown() before close(). >> >> Do this only for the source files for now because the source runs >> multiple threads which could cause races between the two calls. Having >> them together allows us to move them to a centralized place under the >> protection of a lock the next patch. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > > Logically I think we should only need shutdown() when we don't want to > close immediately, or can't for some reason.. Maybe instead of adding > shutdown()s, we can remove some? Wouldn't shutdown() affect what the other end of the socket sees? I thought we used shutdown() before close() as a way to end the connection in a cleaner manner.
On Tue, Aug 15, 2023 at 07:19:43PM -0300, Fabiano Rosas wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Fri, Aug 11, 2023 at 12:08:35PM -0300, Fabiano Rosas wrote: > >> When doing cleanup, we currently close() some of the shared migration > >> files and shutdown() + close() others. Be consistent by always calling > >> shutdown() before close(). > >> > >> Do this only for the source files for now because the source runs > >> multiple threads which could cause races between the two calls. Having > >> them together allows us to move them to a centralized place under the > >> protection of a lock the next patch. > >> > >> Signed-off-by: Fabiano Rosas <farosas@suse.de> > > > > Logically I think we should only need shutdown() when we don't want to > > close immediately, or can't for some reason.. Maybe instead of adding > > shutdown()s, we can remove some? > > Wouldn't shutdown() affect what the other end of the socket sees? I > thought we used shutdown() before close() as a way to end the connection > in a cleaner manner. Not something in my memory. Would you try to avoid shutdown() for whatever we'll close() immediately with next patch? I'd expect no change, but I'm happy to be corrected... Thanks,
diff --git a/migration/migration.c b/migration/migration.c index 7fec57ad7f..4df5ca25c1 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -1175,6 +1175,7 @@ static void migrate_fd_cleanup(MigrationState *s) * critical section won't block for long. */ migration_ioc_unregister_yank_from_file(tmp); + qemu_file_shutdown(tmp); qemu_fclose(tmp); } @@ -1844,6 +1845,7 @@ static void migration_release_dst_files(MigrationState *ms) ms->postcopy_qemufile_src = NULL; } + qemu_file_shutdown(file); qemu_fclose(file); }
When doing cleanup, we currently close() some of the shared migration files and shutdown() + close() others. Be consistent by always calling shutdown() before close(). Do this only for the source files for now because the source runs multiple threads which could cause races between the two calls. Having them together allows us to move them to a centralized place under the protection of a lock the next patch. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/migration.c | 2 ++ 1 file changed, 2 insertions(+)