On 1/10/2024 9:16 PM, Peter Xu wrote:
> On Wed, Jan 10, 2024 at 01:08:41PM -0500, Steven Sistare wrote:
>> On 1/10/2024 2:18 AM, Peter Xu wrote:
>>> On Wed, Dec 13, 2023 at 10:11:32AM -0800, Steve Sistare wrote:
>>>> After calling notifiers, check if an error has been reported via
>>>> migrate_set_error, and halt the migration.
>>>>
>>>> None of the notifiers call migrate_set_error at this time, so no
>>>> functional change.
>>>>
>>>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com>
>>>> ---
>>>> include/migration/misc.h | 2 +-
>>>> migration/migration.c | 26 ++++++++++++++++++++++----
>>>> 2 files changed, 23 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/include/migration/misc.h b/include/migration/misc.h
>>>> index 901d117..231d7e4 100644
>>>> --- a/include/migration/misc.h
>>>> +++ b/include/migration/misc.h
>>>> @@ -65,7 +65,7 @@ MigMode migrate_mode_of(MigrationState *);
>>>> void migration_add_notifier(Notifier *notify,
>>>> void (*func)(Notifier *notifier, void *data));
>>>> void migration_remove_notifier(Notifier *notify);
>>>> -void migration_call_notifiers(MigrationState *s);
>>>> +int migration_call_notifiers(MigrationState *s);
>>>> bool migration_in_setup(MigrationState *);
>>>> bool migration_has_finished(MigrationState *);
>>>> bool migration_has_failed(MigrationState *);
>>>> diff --git a/migration/migration.c b/migration/migration.c
>>>> index d5bfe70..29a9a92 100644
>>>> --- a/migration/migration.c
>>>> +++ b/migration/migration.c
>>>> @@ -1280,6 +1280,8 @@ void migrate_set_state(int *state, int old_state,
>>>> int new_state)
>>>>
>>>> static void migrate_fd_cleanup(MigrationState *s)
>>>> {
>>>> + bool already_failed;
>>>> +
>>>> qemu_bh_delete(s->cleanup_bh);
>>>> s->cleanup_bh = NULL;
>>>>
>>>> @@ -1327,11 +1329,20 @@ static void migrate_fd_cleanup(MigrationState *s)
>>>> MIGRATION_STATUS_CANCELLED);
>>>> }
>>>>
>>>> + already_failed = migration_has_failed(s);
>>>> + if (migration_call_notifiers(s)) {
>>>> + if (!already_failed) {
>>>> + migrate_set_state(&s->state, s->state,
>>>> MIGRATION_STATUS_FAILED);
>>>> + /* Notify again to recover from this late failure. */
>>>> + migration_call_notifiers(s);
>>>> + }
>>>> + }
>>>> +
>>>> if (s->error) {
>>>> /* It is used on info migrate. We can't free it */
>>>> error_report_err(error_copy(s->error));
>>>> }
>>>> - migration_call_notifiers(s);
>>>> +
>>>> block_cleanup_parameters();
>>>> yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>>> }
>>>> @@ -1450,9 +1461,10 @@ void migration_remove_notifier(Notifier *notify)
>>>> }
>>>> }
>>>>
>>>> -void migration_call_notifiers(MigrationState *s)
>>>> +int migration_call_notifiers(MigrationState *s)
>>>> {
>>>> notifier_list_notify(&migration_state_notifiers, s);
>>>> + return (s->error != NULL);
>>>
>>> Exporting more migration_*() functions is pretty ugly to me..
>>
>> I assume you mean migrate_set_error(), which is currently only called from
>> migration/*.c code.
>>
>> Instead, we could define a new function migrate_set_notifier_error(), defined
>> in the new file migration/notifier.h, so we clearly limit the migration
>> functions which can be called from notifiers. (Its implementation just calls
>> migrate_set_error)
>
> Fundementally this allows another .c to change one more field of
> MigrationState (which is ->error) and I still want to avoid it.
>
> I just replied in the other thread, but now with all these in mind I think
> I still prefer not passing in MigrationState* at all. It's already kind of
> abused due to migrate_get_current(), and IMHO it's healthier to limit its
> usage to minimum to cover the core of migration states for migration/ use
> only.
>
> Shrinking or even stop exporting migrate_get_current() is another more
> challenging task, but now what we can do is stop enlarging the direct use
> of MigrationState*.
>
>>
>>> Would it be better to pass in "Error** errp" into each notifiers? That may
>>> need an open coded notifier_list_notify(), breaking the loop if "*errp".
>>>
>>> And the notifier API currently only support one arg.. maybe we should
>>> implement the notifiers ourselves, ideally passing in "(int state, Error
>>> **errp)" instead of "(MigrationState *s)".
>>>
>>> Ideally with that MigrationState* shouldn't be visible outside migration/.
>>
>> I will regret saying this because of the amount of (mechanical) code change
>> involved,
>> but the cleanest solution is:
>
> :)
>
>>
>> * Pass errp to:
>> notifier_with_return_list_notify(NotifierWithReturnList *list, void *data,
>> Error *errp)
>> * Pass errp to the NotifierWithReturn notifier:
>> int (*notify)(NotifierWithReturn *notifier, void *data, Error **errp);
>> * Delete the errp member from struct PostcopyNotifyData and pass errp to the
>> notifier function
>> Ditto for PrecopyNotifyData.
>> * Convert all migration notifiers to NotifierWithReturn
>
> Would you mind changing MigrationState* into an event just like postcopy?
> We don't need to use migration_has_failed() etc., afaict three events
> should be enough for the existing four users, exactly like what postcopy
> does:
>
> - MIG_EVENT_PRECOPY_SETUP
> - MIG_EVENT_PRECOPY_DONE
> - MIG_EVENT_PRECOPY_FAILED
Will do.
> Merging postcopy will be indeed the cleanest. I'm okay if you want to
> leave that for later, but if you'd do that together I'd appreciate that.
Will do.
- Steve
