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

[David Hildenbrand <david@redhat.com>]

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