... 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(-)
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;
}
int migrate_add_blocker_internal(Error *reason, Error **errp)
@@ -4445,6 +4448,7 @@ 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)
diff --git a/migration/migration.h b/migration/migration.h
index ae4ffd3454..66511ce532 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -17,6 +17,7 @@
#include "exec/cpu-common.h"
#include "hw/qdev-core.h"
#include "qapi/qapi-types-migration.h"
+#include "qapi/qmp/json-writer.h"
#include "qemu/thread.h"
#include "qemu/coroutine_int.h"
#include "io/channel.h"
@@ -366,6 +367,9 @@ struct MigrationState {
* This save hostname when out-going migration starts
*/
char *hostname;
+
+ /* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */
+ JSONWriter *vmdesc;
};
void migrate_set_state(int *state, int old_state, int new_state);
diff --git a/migration/savevm.c b/migration/savevm.c
index d8830297e4..ff2b8d0064 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -42,7 +42,6 @@
#include "postcopy-ram.h"
#include "qapi/error.h"
#include "qapi/qapi-commands-migration.h"
-#include "qapi/qmp/json-writer.h"
#include "qapi/clone-visitor.h"
#include "qapi/qapi-builtin-visit.h"
#include "qapi/qmp/qerror.h"
@@ -1189,10 +1188,16 @@ bool qemu_savevm_state_guest_unplug_pending(void)
void qemu_savevm_state_setup(QEMUFile *f)
{
+ MigrationState *ms = migrate_get_current();
SaveStateEntry *se;
Error *local_err = NULL;
int ret;
+ ms->vmdesc = json_writer_new(false);
+ json_writer_start_object(ms->vmdesc, NULL);
+ json_writer_int64(ms->vmdesc, "page_size", qemu_target_page_size());
+ json_writer_start_array(ms->vmdesc, "devices");
+
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;
+
return 0;
}
--
2.39.0* 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(-)
>
> 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;
> }
>
> int migrate_add_blocker_internal(Error *reason, Error **errp)
> @@ -4445,6 +4448,7 @@ 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);
I'm not sure this is happening when you think it is.
I *think* this only happens when qemu quits....
> }
>
> static void migration_instance_init(Object *obj)
> diff --git a/migration/migration.h b/migration/migration.h
> index ae4ffd3454..66511ce532 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -17,6 +17,7 @@
> #include "exec/cpu-common.h"
> #include "hw/qdev-core.h"
> #include "qapi/qapi-types-migration.h"
> +#include "qapi/qmp/json-writer.h"
> #include "qemu/thread.h"
> #include "qemu/coroutine_int.h"
> #include "io/channel.h"
> @@ -366,6 +367,9 @@ struct MigrationState {
> * This save hostname when out-going migration starts
> */
> char *hostname;
> +
> + /* QEMU_VM_VMDESCRIPTION content filled for all non-iterable devices. */
> + JSONWriter *vmdesc;
> };
>
> void migrate_set_state(int *state, int old_state, int new_state);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index d8830297e4..ff2b8d0064 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -42,7 +42,6 @@
> #include "postcopy-ram.h"
> #include "qapi/error.h"
> #include "qapi/qapi-commands-migration.h"
> -#include "qapi/qmp/json-writer.h"
> #include "qapi/clone-visitor.h"
> #include "qapi/qapi-builtin-visit.h"
> #include "qapi/qmp/qerror.h"
> @@ -1189,10 +1188,16 @@ bool qemu_savevm_state_guest_unplug_pending(void)
>
> void qemu_savevm_state_setup(QEMUFile *f)
> {
> + MigrationState *ms = migrate_get_current();
> SaveStateEntry *se;
> Error *local_err = NULL;
> int ret;
>
> + ms->vmdesc = json_writer_new(false);
> + json_writer_start_object(ms->vmdesc, NULL);
> + json_writer_int64(ms->vmdesc, "page_size", qemu_target_page_size());
> + json_writer_start_array(ms->vmdesc, "devices");
> +
> 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.
Dave
> return 0;
> }
>
> --
> 2.39.0
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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
* 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.
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> --
> Thanks,
>
> David / dhildenb
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
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?
--
Peter Xu
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
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
© 2016 - 2026 Red Hat, Inc.