On 25 August 2017 at 15:19, Dr. David Alan Gilbert (git) <dgilb...@redhat.com> wrote: > From: "Dr. David Alan Gilbert" <dgilb...@redhat.com> > > migration_incoming_state_destroy doesn't really destroy, it cleans up. > After a loadvm it's called, but the loadvm command can be run twice, > and so destroying an init-once mutex breaks on the second loadvm. > > Reported-by: Stafford Horne <sho...@gmail.com> > Signed-off-by: Dr. David Alan Gilbert <dgilb...@redhat.com> > --- > migration/migration.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/migration/migration.c b/migration/migration.c > index c3fe0ed9ca..a625551ce5 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -167,7 +167,7 @@ void migration_incoming_state_destroy(void) > mis->from_src_file = NULL; > } > > - qemu_event_destroy(&mis->main_thread_load_event); > + qemu_event_reset(&mis->main_thread_load_event); > }
Is it worth doing more here? For instance: * rename the function to something that better reflects what it's doing * make sure it actually goes back to the state it's in after you first call migration_incoming_get_current() (eg setting mis_current.state, zeroing various fields) * maybe instead of a "get current state" that does a reset if it's not been called before and a "destroy" that does cleanup stuff (like telling the source we've stopped) and also resetting back to clean state, we could structure this with a function that does "give me a clean completely reset state" which you must call first, then use get_current() purely to get the current state with no 'reset on first call' semantics, and finally a "complete" function that just does the "tell source we've stopped" and close resources we no longer need ? PS, in migration_incoming_get_current() we do mis_current.state = MIGRATION_STATUS_NONE; memset(&mis_current, 0, sizeof(MigrationIncomingState)); and the first line there is pointless because the memset blasts zeroes over it anyway. thanks -- PMM