Re: [PATCH v3 2/8] migration/savevm: Prepare vmdesc json writer in qemu_savevm_state_setup()

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* David Hildenbrand (david@redhat.com) wrote:
> ... and store it in the migration state. This is a preparation for
> storing selected vmds's already in qemu_savevm_state_setup().
> 
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  migration/migration.c |  4 ++++
>  migration/migration.h |  4 ++++
>  migration/savevm.c    | 18 ++++++++++++------
>  3 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 52b5d39244..1d33a7efa0 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2170,6 +2170,9 @@ void migrate_init(MigrationState *s)
>      s->vm_was_running = false;
>      s->iteration_initial_bytes = 0;
>      s->threshold_size = 0;
> +
> +    json_writer_free(s->vmdesc);
> +    s->vmdesc = NULL;
>  }
>  
>  int migrate_add_blocker_internal(Error *reason, Error **errp)
> @@ -4445,6 +4448,7 @@ static void migration_instance_finalize(Object *obj)
>      qemu_sem_destroy(&ms->rp_state.rp_sem);
>      qemu_sem_destroy(&ms->postcopy_qemufile_src_sem);
>      error_free(ms->error);
> +    json_writer_free(ms->vmdesc);

I'm not sure this is happening when you think it is.
I *think* this only happens when qemu quits....

>  }
>  
>  static void migration_instance_init(Object *obj)
> diff --git a/migration/migration.h b/migration/migration.h
> index ae4ffd3454..66511ce532 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -17,6 +17,7 @@
>  #include "exec/cpu-common.h"
>  #include "hw/qdev-core.h"
>  #include "qapi/qapi-types-migration.h"
> +#include "qapi/qmp/json-writer.h"
>  #include "qemu/thread.h"
>  #include "qemu/coroutine_int.h"
>  #include "io/channel.h"
> @@ -366,6 +367,9 @@ struct MigrationState {
>       * This save hostname when out-going migration starts
>       */
>      char *hostname;
> +
> +    /* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */
> +    JSONWriter *vmdesc;
>  };
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d8830297e4..ff2b8d0064 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -42,7 +42,6 @@
>  #include "postcopy-ram.h"
>  #include "qapi/error.h"
>  #include "qapi/qapi-commands-migration.h"
> -#include "qapi/qmp/json-writer.h"
>  #include "qapi/clone-visitor.h"
>  #include "qapi/qapi-builtin-visit.h"
>  #include "qapi/qmp/qerror.h"
> @@ -1189,10 +1188,16 @@ bool qemu_savevm_state_guest_unplug_pending(void)
>  
>  void qemu_savevm_state_setup(QEMUFile *f)
>  {
> +    MigrationState *ms = migrate_get_current();
>      SaveStateEntry *se;
>      Error *local_err = NULL;
>      int ret;
>  
> +    ms->vmdesc = json_writer_new(false);
> +    json_writer_start_object(ms->vmdesc, NULL);
> +    json_writer_int64(ms->vmdesc, "page_size", qemu_target_page_size());
> +    json_writer_start_array(ms->vmdesc, "devices");
> +
>      trace_savevm_state_setup();
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          if (!se->ops || !se->ops->save_setup) {
> @@ -1390,15 +1395,12 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>                                                      bool in_postcopy,
>                                                      bool inactivate_disks)
>  {
> -    g_autoptr(JSONWriter) vmdesc = NULL;
> +    MigrationState *ms = migrate_get_current();
> +    JSONWriter *vmdesc = ms->vmdesc;
>      int vmdesc_len;
>      SaveStateEntry *se;
>      int ret;
>  
> -    vmdesc = json_writer_new(false);
> -    json_writer_start_object(vmdesc, NULL);
> -    json_writer_int64(vmdesc, "page_size", qemu_target_page_size());
> -    json_writer_start_array(vmdesc, "devices");
>      QTAILQ_FOREACH(se, &savevm_state.handlers, entry) {
>          ret = vmstate_save(f, se, vmdesc);
>          if (ret) {
> @@ -1433,6 +1435,10 @@ int qemu_savevm_state_complete_precopy_non_iterable(QEMUFile *f,
>          qemu_put_buffer(f, (uint8_t *)json_writer_get(vmdesc), vmdesc_len);
>      }
>  
> +    /* Free it now to detect any inconsistencies. */
> +    json_writer_free(vmdesc);
> +    ms->vmdesc = NULL;

and this only happens when this succesfully exits;  so if this errors
out, and then you retry an outwards migration, I think you've leaked a
writer.

Dave

>      return 0;
>  }
>  
> -- 
> 2.39.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK