Re: [PATCH V1 2/3] migration: notifier error reporting

[Steven Sistare <steven.sistare@oracle.com>]

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.sistare@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