Re: [PATCH V1 1/3] migration: check mode in notifiers
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
Re: [PATCH V1 1/3] migration: check mode in notifiers
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 *); --- - Steve
Re: [PATCH V1 1/3] migration: check mode in notifiers
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. 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? -- Peter Xu
