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 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?

-- 
Thanks,

David / dhildenb