On 1/15/2024 2:37 AM, Peter Xu wrote:
> On Fri, Jan 12, 2024 at 07:05:07AM -0800, Steve Sistare wrote:
>> Move common code for the error path in migrate_fd_connect to a shared
>> fail label. No functional change.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> One nitpick below:
>
>> ---
>> migration/migration.c | 22 +++++++++++-----------
>> 1 file changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index e9914aa..c828ba7 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -3549,6 +3549,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> Error *local_err = NULL;
>> uint64_t rate_limit;
>> bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED;
>> + int expect_state = s->state;
>
> IMHO we can drop this.
>
>>
>> /*
>> * If there's a previous error, free it and prepare for another one.
>> @@ -3603,11 +3604,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> if (migrate_postcopy_ram() || migrate_return_path()) {
>> if (open_return_path_on_source(s)) {
>> error_setg(&local_err, "Unable to open return-path for postcopy");
>> - migrate_set_state(&s->state, s->state, MIGRATION_STATUS_FAILED);
>> - migrate_set_error(s, local_err);
>> - error_report_err(local_err);
>> - migrate_fd_cleanup(s);
>> - return;
>> + goto fail;
>> }
>> }
>>
>> @@ -3629,12 +3626,8 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> }
>>
>> if (multifd_save_setup(&local_err) != 0) {
>> - migrate_set_error(s, local_err);
>> - error_report_err(local_err);
>> - migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
>> - MIGRATION_STATUS_FAILED);
>> - migrate_fd_cleanup(s);
>> - return;
>> + expect_state = MIGRATION_STATUS_SETUP;
>
> Then drop this.
>
>> + goto fail;
>> }
>>
>> if (migrate_background_snapshot()) {
>> @@ -3645,6 +3638,13 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
>> migration_thread, s, QEMU_THREAD_JOINABLE);
>> }
>> s->migration_thread_running = true;
>> + return;
>> +
>> +fail:
>> + migrate_set_error(s, local_err);
>> + migrate_set_state(&s->state, expect_state, MIGRATION_STATUS_FAILED);
>
> Then constantly use s->state.
Yes, if you are OK with it, I also prefer to simplify here.
- Steve
>> + error_report_err(local_err);
>> + migrate_fd_cleanup(s);
>> }
>>
>> static void migration_class_init(ObjectClass *klass, void *data)
>> --
>> 1.8.3.1
>>
>