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

[David Hildenbrand <david@redhat.com>]

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?

-- 
Thanks,

David / dhildenb