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(+)

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

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

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,

-- 
Peter Xu