[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_t
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] migration: Reset rather than destroy main_thread_load_event |
Date: |
Fri, 25 Aug 2017 16:28:26 +0100 |
On 25 August 2017 at 15:19, Dr. David Alan Gilbert (git)
<address@hidden> wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> 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 <address@hidden>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
> 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