Re: [PATCH v3 06/25] migration: Use migrate_mode() to query for cpr-transfer

[Fabiano Rosas <farosas@suse.de>]

Prasad Pandit <ppandit@redhat.com> writes:

> Hi,
>
> On Fri, 9 Jan 2026 at 18:12, Fabiano Rosas <farosas@suse.de> wrote:
>> cpr_set_incoming_mode() is only called on the target side, so
>> migrate_mode() on the source side is the same as s->parameters.mode.
>
> * This message is a little confusing, as the connection between
> cpr_set_incoming_mode() and migrate_mode() is not evident. IIUC,
> during CPR migration mode is not set explicitly on the source side, so
> cpr_get_incoming_mode() returns none, so migrate_mode() sets it to
> s->parameters.mode and returns it. Ideally it should be set to
> s->parameters.mode.  /* I wonder why we need such differention between
> incoming and outgoing modes. */
>

Good question. I had to go check. It's subtle and not properly
documented: cpr_state_load() is called before current_migration
exists. There is no parameters.mode at that point.

>> Use the function to reduce explicit access to s->parameters, we have
>> options.c for that.
>>
>> Cc: Mark Kanda <mark.kanda@oracle.com>
>> Cc: Ben Chaney <bchaney@akamai.com>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/cpr-exec.c  |  2 +-
>>  migration/migration.c | 27 +++++++++++++--------------
>>  migration/migration.h |  5 ++---
>>  3 files changed, 16 insertions(+), 18 deletions(-)
>>
>> diff --git a/migration/cpr-exec.c b/migration/cpr-exec.c
>> index da287d8031..e315a30f92 100644
>> --- a/migration/cpr-exec.c
>> +++ b/migration/cpr-exec.c
>> @@ -164,7 +164,7 @@ static void cpr_exec_cb(void *opaque)
>>      err = NULL;
>>
>>      /* Note, we can go from state COMPLETED to FAILED */
>
>  /* Going from COMPLETED -> FAILED says something not right here,
> maybe we are prematurely moving to COMPLETED state. */
>

I agree there's a dissonance, but there is no "after" to counterpart
this "prematurely".

cpr-exec happens after migration completes, it needs to receive the
MIG_EVENT_PRECOPY_DONE completion notifier. But then there's a slight
chance that the execvp() may fail.

If it fails, then migration is naturally FAILED and the
MIG_EVENT_PRECOPY_FAILED notifiers are called below.

If it succeeds, then it's another process image, s->state is no more.

There might also be considerations from the management layer side in
seeing the completion event and from the device migration code in seeing
the completion notifier, I'm not sure.

>> -    migration_call_notifiers(s, MIG_EVENT_PRECOPY_FAILED, NULL);
>> +    migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
>>
>>      if (!migration_block_activate(&err)) {
>>          /* error was already reported */
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 4af5baad59..388e0be5a2 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1534,7 +1534,7 @@ static void migration_cleanup(MigrationState *s)
>>      }
>>      type = migration_has_failed(s) ? MIG_EVENT_PRECOPY_FAILED :
>>                                       MIG_EVENT_PRECOPY_DONE;
>> -    migration_call_notifiers(s, type, NULL);
>> +    migration_call_notifiers(type, NULL);
>>      yank_unregister_instance(MIGRATION_YANK_INSTANCE);
>>  }
>>
>> @@ -1696,10 +1696,9 @@ void migration_remove_notifier(NotifierWithReturn *notify)
>>      }
>>  }
>>
>> -int migration_call_notifiers(MigrationState *s, MigrationEventType type,
>> -                             Error **errp)
>> +int migration_call_notifiers(MigrationEventType type, Error **errp)
>>  {
>> -    MigMode mode = s->parameters.mode;
>> +    MigMode mode = migrate_mode();
>>      MigrationEvent e;
>>      NotifierWithReturn *notifier;
>>      GSList *elem, *next;
>> @@ -1780,9 +1779,9 @@ bool migration_thread_is_self(void)
>>      return qemu_thread_is_self(&s->thread);
>>  }
>>
>> -bool migrate_mode_is_cpr(MigrationState *s)
>> +bool migrate_mode_is_cpr(void)
>>  {
>> -    MigMode mode = s->parameters.mode;
>> +    MigMode mode = migrate_mode();
>>      return mode == MIG_MODE_CPR_REBOOT ||
>>             mode == MIG_MODE_CPR_TRANSFER ||
>>             mode == MIG_MODE_CPR_EXEC;
>> @@ -2136,7 +2135,7 @@ static bool migrate_prepare(MigrationState *s, bool resume, Error **errp)
>>          }
>>      }
>>
>> -    if (migrate_mode_is_cpr(s)) {
>> +    if (migrate_mode_is_cpr()) {
>>          const char *conflict = NULL;
>>
>>          if (migrate_postcopy()) {
>> @@ -2252,7 +2251,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>>          return;
>>      }
>>
>> -    if (s->parameters.mode == MIG_MODE_CPR_TRANSFER && !cpr_channel) {
>> +    if (migrate_mode() == MIG_MODE_CPR_TRANSFER && !cpr_channel) {
>>          error_setg(errp, "missing 'cpr' migration channel");
>>          return;
>>      }
>> @@ -2277,7 +2276,7 @@ void qmp_migrate(const char *uri, bool has_channels,
>>       * in which case the target will not listen for the incoming migration
>>       * connection, so qmp_migrate_finish will fail to connect, and then recover.
>>       */
>> -    if (s->parameters.mode == MIG_MODE_CPR_TRANSFER) {
>> +    if (migrate_mode() == MIG_MODE_CPR_TRANSFER) {
>
> * Shouldn't we use migrate_mode_is_cpr() to avoid this comparison?
>

There are other CPR modes that I don't think we want to match here, only
cpr-transfer.

>>          migrate_hup_add(s, cpr_state_ioc(), (GSourceFunc)qmp_migrate_finish_cb,
>>                          QAPI_CLONE(MigrationAddress, addr));
>>
>> @@ -2852,7 +2851,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>>       * at the transition to postcopy and after the device state; in particular
>>       * spice needs to trigger a transition now
>>       */
>> -    migration_call_notifiers(ms, MIG_EVENT_PRECOPY_DONE, NULL);
>> +    migration_call_notifiers(MIG_EVENT_PRECOPY_DONE, NULL);
>>
>>      migration_downtime_end(ms);
>>
>> @@ -2901,7 +2900,7 @@ fail:
>>          migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
>>      }
>>      migration_block_activate(NULL);
>> -    migration_call_notifiers(ms, MIG_EVENT_PRECOPY_FAILED, NULL);
>> +    migration_call_notifiers(MIG_EVENT_PRECOPY_FAILED, NULL);
>>      bql_unlock();
>>      return -1;
>>  }
>> @@ -3003,7 +3002,7 @@ static int migration_completion_precopy(MigrationState *s)
>>
>>      bql_lock();
>>
>> -    if (!migrate_mode_is_cpr(s)) {
>> +    if (!migrate_mode_is_cpr()) {
>>          ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
>>          if (ret < 0) {
>>              goto out_unlock;
>> @@ -4049,7 +4048,7 @@ void migration_connect(MigrationState *s, Error *error_in)
>>          rate_limit = migrate_max_bandwidth();
>>
>>          /* Notify before starting migration thread */
>> -        if (migration_call_notifiers(s, MIG_EVENT_PRECOPY_SETUP, &local_err)) {
>> +        if (migration_call_notifiers(MIG_EVENT_PRECOPY_SETUP, &local_err)) {
>>              goto fail;
>>          }
>>      }
>> @@ -4085,7 +4084,7 @@ void migration_connect(MigrationState *s, Error *error_in)
>>          return;
>>      }
>>
>> -    if (migrate_mode_is_cpr(s)) {
>> +    if (migrate_mode_is_cpr()) {
>>          ret = migration_stop_vm(s, RUN_STATE_FINISH_MIGRATE);
>>          if (ret < 0) {
>>              error_setg(&local_err, "migration_stop_vm failed, error %d", -ret);
>> diff --git a/migration/migration.h b/migration/migration.h
>> index ccc4e536a5..8b55d4741a 100644
>> --- a/migration/migration.h
>> +++ b/migration/migration.h
>> @@ -538,8 +538,7 @@ bool migrate_has_error(MigrationState *s);
>>
>>  void migration_connect(MigrationState *s, Error *error_in);
>>
>> -int migration_call_notifiers(MigrationState *s, MigrationEventType type,
>> -                             Error **errp);
>> +int migration_call_notifiers(MigrationEventType type, Error **errp);
>>
>>  int migrate_init(MigrationState *s, Error **errp);
>>  bool migration_is_blocked(Error **errp);
>> @@ -548,7 +547,7 @@ bool migration_in_postcopy(void);
>>  bool migration_postcopy_is_alive(MigrationStatus state);
>>  MigrationState *migrate_get_current(void);
>>  bool migration_has_failed(MigrationState *);
>> -bool migrate_mode_is_cpr(MigrationState *);
>> +bool migrate_mode_is_cpr(void);
>>
>>  uint64_t ram_get_total_transferred_pages(void);
>>
>> --
>
> * Otherwise change looks okay. Needs minor fix above.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>
> Thank you.
> ---
>    - Prasad