Re: [PATCH qemu v2 6/9] savevm-async: free migration vmdesc

[Fiona Ebner <f.ebner@proxmox.com>]

Am 07.09.26 um 11:48 AM schrieb Erik Fastermann:
>  diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> @@ -149,10 +150,10 @@ index 0222d5ea6e..90d62d5723 100644
>   ), gnutls, zlib)
>  diff --git a/migration/savevm-async.c b/migration/savevm-async.c
>  new file mode 100644
> -index 0000000000..cab08643ad
> +index 0000000000..4f44c2f12d
>  --- /dev/null
>  +++ b/migration/savevm-async.c
> -@@ -0,0 +1,600 @@
> +@@ -0,0 +1,608 @@
>  +#include "qemu/osdep.h"
>  +#include "migration/channel-savevm-async.h"
>  +#include "migration/migration.h"
> @@ -261,6 +262,15 @@ index 0000000000..cab08643ad
>  +    return info;
>  +}
>  +
> ++static void migration_state_cleanup(MigrationState *ms, bool failed)
> ++{
> ++    migrate_set_state(&ms->state, MIGRATION_STATUS_SETUP,
> ++        failed ? MIGRATION_STATUS_FAILED : MIGRATION_STATUS_COMPLETED);
> ++    ms->to_dst_file = NULL;
> ++    /* see migration_cleanup_json_writer */
> ++    g_clear_pointer(&ms->vmdesc, json_writer_free);
> ++}
> ++
>  +static int save_snapshot_cleanup(void)
>  +{
>  +    int ret = 0;
> @@ -359,10 +369,7 @@ index 0000000000..cab08643ad
>  +    DPRINTF("timing: process_savevm_finalize (state saving) took %ld ms\n",
>  +        qemu_clock_get_ms(QEMU_CLOCK_REALTIME) - start_time);
>  +
> -+    /* clear migration state */
> -+    migrate_set_state(&ms->state, MIGRATION_STATUS_SETUP,
> -+        ret || aborted ? MIGRATION_STATUS_FAILED : MIGRATION_STATUS_COMPLETED);
> -+    ms->to_dst_file = NULL;
> ++    migration_state_cleanup(ms, ret || aborted);
>  +
>  +    /*
>  +     * Same as in migration_iteration_finish(): saving RAM might've turned on CPU throttling for
> @@ -618,6 +625,8 @@ index 0000000000..cab08643ad
>  +    return;
>  +
>  +fail:
> ++    migration_state_cleanup(ms, true);

While fine in practice, because the implementations handle it, it reads
wrong to have the migrate_set_state() and g_clear_pointer(&ms->vmdesc,
...) calls when we fail before having a successful migrate_init(). Can
we guard this call based on whether we already have a successful init?

> ++
>  +    savevm_cleanup_iothread();
>  +    save_snapshot_error("setup failed");
>  +}