* David Hildenbrand (da...@redhat.com) wrote:
> On 12.01.23 18:43, Dr. David Alan Gilbert wrote:
> > * David Hildenbrand (da...@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 <da...@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.
Reviewed-by: Dr. David Alan Gilbert <dgilb...@redhat.com>
> --
> Thanks,
>
> David / dhildenb
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
