[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH V1 1/3] migration: check mode in notifiers
From: |
Peter Xu |
Subject: |
Re: [PATCH V1 1/3] migration: check mode in notifiers |
Date: |
Thu, 11 Jan 2024 09:45:47 +0800 |
On Wed, Jan 10, 2024 at 01:08:01PM -0500, Steven Sistare wrote:
> On 1/10/2024 2:09 AM, Peter Xu wrote:
> > On Wed, Dec 13, 2023 at 10:11:31AM -0800, Steve Sistare wrote:
> >> The existing notifiers should only apply to normal mode.
> >>
> >> No functional change.
> >
> > Instead of adding such check in every notifier, why not make CPR a separate
> > list of notifiers? Just like the blocker lists.
>
> Sure. I proposed minimal changes in this current series, but extending the
> api to take migration mode would be nicer.
>
> > Aside of this patch, I just started to look at this "notifier" code, I
> > really don't think we should pass in MigrationState* into the notifiers.
> > IIUC we only need the "state" as an enum. Then with two separate
> > registers, the device code knows the migration mode.
> >
> > What do you think?
>
> If we pass state, the notifier must either compare to enum values such as
> MIGRATION_STATUS_COMPLETED instead of calling migration_has_finished(s), or
> we must define new accessors such as migration_state_is_finished(state).
>
> IMO passing MigrationState is the best approach.
> MigrationState is an incomplete type in most notifiers, and the client can
> pass it to a limited set of accessors to get more information -- exactly what
> we want to hide migration internals. However, we could further limit the
> allowed accessors, eg move these to a new file "include/migration/notifier.h".
>
> ----------------------------------------
> #include "qemu/notify.h"
> void migration_add_notifier(Notifier *notify,
> void (*func)(Notifier *notifier, void *data));
> void migration_remove_notifier(Notifier *notify);
> bool migration_is_active(MigrationState *);
> bool migration_in_setup(MigrationState *);
> bool migration_has_finished(MigrationState *);
> bool migration_has_failed(MigrationState *);
> -----------------------------------------------
Yes this also sounds good. Thanks,
--
Peter Xu