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 (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.
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