* Peter Maydell (peter.mayd...@linaro.org) wrote: > 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?
In the longer-term, yes; it seemed right just to get this bug fixed first though. > 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 ? Yes; an init/current/clean does make sense; one of my comments on one of Peter's patchsets was to point out the migration_incoming_get_current isn't thread safe if you're not sure you've already called it, so it could do with fixing. There has also been a few things that have wanted to gather stats that are available after the end of an incoming migration; so we don't really want to destroy that state, we just want to get rid of anything temporary. You could argue that this thread_load_event is best init'd at the start of the incoming migration and then destroyed at the end. > 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. Hmm yes. Dave > thanks > -- PMM -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
