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

[David Hildenbrand]

On 13.01.23 14:01, David Hildenbrand wrote:
> On 12.01.23 23:06, Peter Xu wrote:
> > On Thu, Jan 12, 2023 at 06:40:00PM +0000, Dr. David Alan Gilbert wrote:
> > > * David Hildenbrand (david@redhat.com) wrote:
> > > > On 12.01.23 18:43, Dr. David Alan Gilbert wrote:
> > > > > * 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(-)
> > > >
> > > > [1]
> > > >
> > > > > > 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;
> > > > > >     }
> > > >
> > > > [...]
> > > >
> > > > > >         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.
> > > >
> > > > Shouldn't the change [1] to migrate_init() cover that?
> > >
> > > Hmm OK, yes it does - I guess it does mean you keep the allocation
> > > around for a bit longer, but that's OK in practice since normally you'll
> > > be quitting soon.
> >
> > Instead of json_writer_free() here and there, how about free it in
> > migrate_fd_cleanup() once and for all?
>
> Sure, if that works. I assume I can get rid of the migrate_init() and
> migration_instance_finalize() change then, correct?

Yeah, that should be much better and matches how we handle the other 
members:

diff --git a/migration/migration.c b/migration/migration.c
index 1d33a7efa0..fcd2f20d7c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1903,6 +1903,8 @@ static void migrate_fd_cleanup(MigrationState *s)

     g_free(s->hostname);
     s->hostname = NULL;
+    json_writer_free(s->vmdesc);
+    s->vmdesc = NULL;

     qemu_savevm_state_cleanup();

@@ -2170,9 +2172,6 @@ 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)
@@ -4448,7 +4447,6 @@ 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);
 }

 static void migration_instance_init(Object *obj)


--
Thanks,

David / dhildenb