Re: [PATCH 2/3] migration: Simplify calling of await_return_path_close_on_source

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Jul 28, 2023 at 09:15:15AM -0300, Fabiano Rosas wrote:
>> We're about to reuse this function so move the 'rp_thread_created'
>> check into it and remove the redundant tracing and comment.
>> 
>> Add a new tracepoint akin to what is already done at
>> migration_completion().
>> 
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>  migration/migration.c  | 21 +++++++--------------
>>  migration/trace-events |  3 +--
>>  2 files changed, 8 insertions(+), 16 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 051067f8c5..d6f4470265 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -2038,6 +2038,10 @@ static int open_return_path_on_source(MigrationState *ms,
>>  /* Returns 0 if the RP was ok, otherwise there was an error on the RP */
>>  static int await_return_path_close_on_source(MigrationState *ms)
>>  {
>> +    if (!ms->rp_state.rp_thread_created) {
>> +        return 0;
>> +    }
>> +
>>      /*
>>       * If this is a normal exit then the destination will send a SHUT and the
>>       * rp_thread will exit, however if there's an error we need to cause
>> @@ -2350,20 +2354,9 @@ static void migration_completion(MigrationState *s)
>>          goto fail;
>>      }
>>  
>> -    /*
>> -     * If rp was opened we must clean up the thread before
>> -     * cleaning everything else up (since if there are no failures
>> -     * it will wait for the destination to send it's status in
>> -     * a SHUT command).
>> -     */
>> -    if (s->rp_state.rp_thread_created) {
>> -        int rp_error;
>> -        trace_migration_return_path_end_before();
>> -        rp_error = await_return_path_close_on_source(s);
>> -        trace_migration_return_path_end_after(rp_error);
>> -        if (rp_error) {
>> -            goto fail;
>> -        }
>> +    if (await_return_path_close_on_source(s)) {
>> +        trace_migration_completion_rp_err();
>> +        goto fail;
>>      }
>>  
>>      if (qemu_file_get_error(s->to_dst_file)) {
>> diff --git a/migration/trace-events b/migration/trace-events
>> index 5259c1044b..33a69064ca 100644
>> --- a/migration/trace-events
>> +++ b/migration/trace-events
>> @@ -157,13 +157,12 @@ migrate_pending_estimate(uint64_t size, uint64_t pre, uint64_t post) "estimate p
>>  migrate_send_rp_message(int msg_type, uint16_t len) "%d: len %d"
>>  migrate_send_rp_recv_bitmap(char *name, int64_t size) "block '%s' size 0x%"PRIi64
>>  migration_completion_file_err(void) ""
>> +migration_completion_rp_err(void) ""
>>  migration_completion_vm_stop(int ret) "ret %d"
>>  migration_completion_postcopy_end(void) ""
>>  migration_completion_postcopy_end_after_complete(void) ""
>>  migration_rate_limit_pre(int ms) "%d ms"
>>  migration_rate_limit_post(int urgent) "urgent: %d"
>> -migration_return_path_end_before(void) ""
>> -migration_return_path_end_after(int rp_error) "%d"
>>  migration_thread_after_loop(void) ""
>>  migration_thread_file_err(void) ""
>>  migration_thread_setup_complete(void) ""
>> -- 
>> 2.35.3
>> 
>
> Just in case someone may still want to see the old
> trace_migration_return_path_end_before() tracepoint, maybe just move them
> all over?

They are made redundant by
trace_await_return_path_close_on_source_joining() and
trace_await_return_path_close_on_source_close() which already exist in
the function.