[v1] allow cpr-reboot for vfio

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

Posted by Steve Sistare 1 year, 11 months ago

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);
 }
 
 bool migration_in_setup(MigrationState *s)
@@ -2520,7 +2532,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
      * spice needs to trigger a transition now
      */
     ms->postcopy_after_devices = true;
-    migration_call_notifiers(ms);
+    if (migration_call_notifiers(ms)) {
+        goto fail;
+    }
 
     migration_downtime_end(ms);
 
@@ -3589,7 +3603,11 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         rate_limit = migrate_max_bandwidth();
 
         /* Notify before starting migration thread */
-        migration_call_notifiers(s);
+        if (migration_call_notifiers(s)) {
+            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
+            migrate_fd_cleanup(s);
+            return;
+        }
     }
 
     migration_rate_set(rate_limit);
-- 
1.8.3.1

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

Posted by Peter Xu 1 year, 10 months ago

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

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/.

Thanks,

>  }
>  
>  bool migration_in_setup(MigrationState *s)
> @@ -2520,7 +2532,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>       * spice needs to trigger a transition now
>       */
>      ms->postcopy_after_devices = true;
> -    migration_call_notifiers(ms);
> +    if (migration_call_notifiers(ms)) {
> +        goto fail;
> +    }
>  
>      migration_downtime_end(ms);
>  
> @@ -3589,7 +3603,11 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>          rate_limit = migrate_max_bandwidth();
>  
>          /* Notify before starting migration thread */
> -        migration_call_notifiers(s);
> +        if (migration_call_notifiers(s)) {
> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
> +            migrate_fd_cleanup(s);
> +            return;
> +        }
>      }
>  
>      migration_rate_set(rate_limit);
> -- 
> 1.8.3.1
> 

-- 
Peter Xu

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

Posted by Steven Sistare 1 year, 10 months ago

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)

> 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

- Steve

>>  }
>>  
>>  bool migration_in_setup(MigrationState *s)
>> @@ -2520,7 +2532,9 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>>       * spice needs to trigger a transition now
>>       */
>>      ms->postcopy_after_devices = true;
>> -    migration_call_notifiers(ms);
>> +    if (migration_call_notifiers(ms)) {
>> +        goto fail;
>> +    }
>>  
>>      migration_downtime_end(ms);
>>  
>> @@ -3589,7 +3603,11 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>>          rate_limit = migrate_max_bandwidth();
>>  
>>          /* Notify before starting migration thread */
>> -        migration_call_notifiers(s);
>> +        if (migration_call_notifiers(s)) {
>> +            migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> +            migrate_fd_cleanup(s);
>> +            return;
>> +        }
>>      }
>>  
>>      migration_rate_set(rate_limit);
>> -- 
>> 1.8.3.1
>>
>

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

Posted by Peter Xu 1 year, 10 months ago

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

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.

Thanks,

-- 
Peter Xu

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

Posted by Steven Sistare 1 year, 10 months ago

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

[PATCH V1 1/3] migration: check mode in notifiers
[PATCH V1 2/3] migration: notifier error reporting
[PATCH V1 3/3] vfio: allow cpr-reboot migration if suspended