If local_err is set, migration_connect_error_propagate() will be
called and that function already has a state transtion from SETUP to
FAILED.

Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 9d1bf5d276..c45393f40e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2326,8 +2326,6 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
         file_start_outgoing_migration(s, &addr->u.file, &local_err);
     } else {
         error_setg(&local_err, "uri is not a valid migration protocol");
-        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                          MIGRATION_STATUS_FAILED);
     }
 
     if (local_err) {
-- 
2.51.0

On Fri, 9 Jan 2026 at 18:11, Fabiano Rosas <farosas@suse.de> wrote:
> If local_err is set, migration_connect_error_propagate() will be
> called and that function already has a state transtion from SETUP to
> FAILED.

* transtion -> transition.

> diff --git a/migration/migration.c b/migration/migration.c
> index 9d1bf5d276..c45393f40e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2326,8 +2326,6 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>          file_start_outgoing_migration(s, &addr->u.file, &local_err);
>      } else {
>          error_setg(&local_err, "uri is not a valid migration protocol");
> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
> -                          MIGRATION_STATUS_FAILED);
>      }

* Maybe we could remove this last '} else {' block altogether? The
'addr->transport == ' check could be moved to the
migrate_transport_compatible() function (OR near there), which is
called after addr = channel->addr.

* Saying that - "uri is invalid" - in qmp_migrate_finish() raises a
question - how did we reach till _migrate_finish() if uri was invalid?
Is qmp_migrate_finish() called by libvirtd(8) OR external users via
QMP? If not, if it is an internal only function to just start
migration, it could be renamed appropriately without qmp_ prefix -
migrate_start() OR begin_migration() something to the effect that says
it starts migration, rather than finish it. migrate_finish() function
calling  *_start_outgoing_migration() reads contradictory/opposite.

(I'm going through the rest of the patches in this series.)

Thank you.
---
  - Prasad

Prasad Pandit <ppandit@redhat.com> writes:

> On Fri, 9 Jan 2026 at 18:11, Fabiano Rosas <farosas@suse.de> wrote:
>> If local_err is set, migration_connect_error_propagate() will be
>> called and that function already has a state transtion from SETUP to
>> FAILED.
>
> * transtion -> transition.
>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 9d1bf5d276..c45393f40e 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2326,8 +2326,6 @@ static void qmp_migrate_finish(MigrationAddress *addr, bool resume_requested,
>>          file_start_outgoing_migration(s, &addr->u.file, &local_err);
>>      } else {
>>          error_setg(&local_err, "uri is not a valid migration protocol");
>> -        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>> -                          MIGRATION_STATUS_FAILED);
>>      }
>
> * Maybe we could remove this last '} else {' block altogether? The
> 'addr->transport == ' check could be moved to the
> migrate_transport_compatible() function (OR near there), which is
> called after addr = channel->addr.
>

This needs to be here (i.e. in two parts) because of the CPR transfer
flow.

> * Saying that - "uri is invalid" - in qmp_migrate_finish() raises a
> question - how did we reach till _migrate_finish() if uri was invalid?

The split in qmp_migrate/qmp_migrate_finish is so that the connection
and migration-start steps can be separated by the CPR transfer hangup
signal. For regular migration it's still as if this were a single
routine.

> Is qmp_migrate_finish() called by libvirtd(8) OR external users via
> QMP? If not, if it is an internal only function to just start
> migration, it could be renamed appropriately without qmp_ prefix -
> migrate_start() OR begin_migration() something to the effect that says
> it starts migration, rather than finish it. migrate_finish() function
> calling  *_start_outgoing_migration() reads contradictory/opposite.
>

Yes, patch 24 at the end of the series takes care of this. I first need
to put some things in place so that we can reuse the connection code for
both the regular migration and cpr-transfer.

> (I'm going through the rest of the patches in this series.)
>
> Thank you.
> ---
>   - Prasad