[PATCH v4 1/4] migration/savevm.c: reorder usage and assertion of mis->from_src_file

Sergei Heifetz posted 4 patches 3 weeks, 6 days ago
Maintainers: "Marc-André Lureau" <marcandre.lureau@redhat.com>, Ani Sinha <anisinha@redhat.com>, Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>, Paolo Bonzini <pbonzini@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
[PATCH v4 1/4] migration/savevm.c: reorder usage and assertion of mis->from_src_file
Posted by Sergei Heifetz 3 weeks, 6 days ago
Reorder the code so the assertion of mis->from_src_file occurs before
the call to migration_ioc_unregister_yank_from_file, which dereferences
it in qemu_file_get_ioc.

Fixes: 39675ffffb3394 ("migration: Move the yank unregister of channel_close out")
Signed-off-by: Sergei Heifetz <heifetz@yandex-team.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
---
 migration/savevm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/savevm.c b/migration/savevm.c
index 3dc812a7bbb..930a3391e35 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2885,13 +2885,14 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
 
     assert(migrate_postcopy_ram());
 
+    assert(mis->from_src_file);
+
     /*
      * Unregister yank with either from/to src would work, since ioc behind it
      * is the same
      */
     migration_ioc_unregister_yank_from_file(mis->from_src_file);
 
-    assert(mis->from_src_file);
     qemu_file_shutdown(mis->from_src_file);
     qemu_fclose(mis->from_src_file);
     mis->from_src_file = NULL;
-- 
2.34.1
Re: [PATCH v4 1/4] migration/savevm.c: reorder usage and assertion of mis->from_src_file
Posted by Prasad Pandit 3 weeks, 5 days ago
On Wed, 11 Mar 2026 at 18:47, Sergei Heifetz <heifetz@yandex-team.com> wrote:
> Reorder the code so the assertion of mis->from_src_file occurs before
> the call to migration_ioc_unregister_yank_from_file, which dereferences
> it in qemu_file_get_ioc.
>
> Fixes: 39675ffffb3394 ("migration: Move the yank unregister of channel_close out")
> Signed-off-by: Sergei Heifetz <heifetz@yandex-team.com>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  migration/savevm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 3dc812a7bbb..930a3391e35 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2885,13 +2885,14 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
>
>      assert(migrate_postcopy_ram());
>
> +    assert(mis->from_src_file);
> +
>      /*
>       * Unregister yank with either from/to src would work, since ioc behind it
>       * is the same
>       */
>      migration_ioc_unregister_yank_from_file(mis->from_src_file);
>
> -    assert(mis->from_src_file);
>      qemu_file_shutdown(mis->from_src_file);
>      qemu_fclose(mis->from_src_file);
>      mis->from_src_file = NULL;

*  Change looks okay. But is it really possible that we reach the
Postcopy pause step with mis->from/to_src_file = NULL?
    Maybe we could move the following 'assert(mis->to_src_file);' to
the top too? Make it fail early before any call(s) further.
===
diff --git a/migration/savevm.c b/migration/savevm.c
index 197c89e0e6..18264feaac 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2874,6 +2874,8 @@ static bool
postcopy_pause_incoming(MigrationIncomingState *mis)
     trace_postcopy_pause_incoming();

     assert(migrate_postcopy_ram());
+    assert(mis->from_src_file);
+    assert(mis->to_src_file);

     /*
      * Unregister yank with either from/to src would work, since ioc behind it
@@ -2881,12 +2883,10 @@ static bool
postcopy_pause_incoming(MigrationIncomingState *mis)
      */
     migration_ioc_unregister_yank_from_file(mis->from_src_file);

-    assert(mis->from_src_file);
     qemu_file_shutdown(mis->from_src_file);
     qemu_fclose(mis->from_src_file);
     mis->from_src_file = NULL;

-    assert(mis->to_src_file);
     qemu_file_shutdown(mis->to_src_file);
     qemu_mutex_lock(&mis->rp_mutex);
     qemu_fclose(mis->to_src_file);
===

Thank you.
---
  - Prasad
Re: [PATCH v4 1/4] migration/savevm.c: reorder usage and assertion of mis->from_src_file
Posted by Fabiano Rosas 3 weeks, 6 days ago
Sergei Heifetz <heifetz@yandex-team.com> writes:

> Reorder the code so the assertion of mis->from_src_file occurs before
> the call to migration_ioc_unregister_yank_from_file, which dereferences
> it in qemu_file_get_ioc.
>
> Fixes: 39675ffffb3394 ("migration: Move the yank unregister of channel_close out")
> Signed-off-by: Sergei Heifetz <heifetz@yandex-team.com>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  migration/savevm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 3dc812a7bbb..930a3391e35 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2885,13 +2885,14 @@ static bool postcopy_pause_incoming(MigrationIncomingState *mis)
>  
>      assert(migrate_postcopy_ram());
>  
> +    assert(mis->from_src_file);
> +
>      /*
>       * Unregister yank with either from/to src would work, since ioc behind it
>       * is the same
>       */
>      migration_ioc_unregister_yank_from_file(mis->from_src_file);
>  
> -    assert(mis->from_src_file);
>      qemu_file_shutdown(mis->from_src_file);
>      qemu_fclose(mis->from_src_file);
>      mis->from_src_file = NULL;

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